From 5a7408a919e1128e75f23c2598926814685928f6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 22 May 2026 16:34:36 +0200 Subject: [PATCH] fix(github): improve GitHub App setup and installation flow - resolve the GitHub App by a stable identifier during installation callbacks so installing and re-installing keeps working over the full lifetime of the App - verify the installation id received from the callback against the GitHub API before persisting it - support re-installing an already configured GitHub App instead of blocking it - require an authenticated session and rate limit the setup callback routes - extend manifest setup state validity to match GitHub's manifest code lifetime Adds feature coverage for the GitHub App setup and installation callbacks. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/Http/Controllers/Webhook/Github.php | 164 ++++++++--- app/Livewire/Source/Github/Change.php | 23 ++ .../livewire/source/github/change.blade.php | 7 +- routes/webhooks.php | 7 +- .../Security/GithubAppSetupCallbackTest.php | 257 ++++++++++++++++++ 5 files changed, 413 insertions(+), 45 deletions(-) create mode 100644 tests/Feature/Security/GithubAppSetupCallbackTest.php diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 84d7b81f0..b481f4a67 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -12,6 +12,7 @@ use App\Models\GithubApp; use App\Models\PrivateKey; use Exception; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; use Illuminate\Support\Str; use Visus\Cuid2\Cuid2; @@ -452,53 +453,136 @@ class Github extends Controller public function redirect(Request $request) { - try { - $code = $request->get('code'); - $state = $request->get('state'); - $github_app = GithubApp::where('uuid', $state)->firstOrFail(); - $api_url = data_get($github_app, 'api_url'); - $data = Http::withBody(null)->accept('application/vnd.github+json')->post("$api_url/app-manifests/$code/conversions")->throw()->json(); - $id = data_get($data, 'id'); - $slug = data_get($data, 'slug'); - $client_id = data_get($data, 'client_id'); - $client_secret = data_get($data, 'client_secret'); - $private_key = data_get($data, 'pem'); - $webhook_secret = data_get($data, 'webhook_secret'); - $private_key = PrivateKey::create([ - 'name' => "github-app-{$slug}", - 'private_key' => $private_key, - 'team_id' => $github_app->team_id, - 'is_git_related' => true, - ]); - $github_app->name = $slug; - $github_app->app_id = $id; - $github_app->client_id = $client_id; - $github_app->client_secret = $client_secret; - $github_app->webhook_secret = $webhook_secret; - $github_app->private_key_id = $private_key->id; - $github_app->save(); + $code = (string) $request->query('code', ''); + abort_if(blank($code), 422, 'Missing GitHub App manifest code.'); - return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); - } catch (Exception $e) { - return handleError($e); - } + $github_app = $this->consumeGithubAppSetupState( + request: $request, + state: (string) $request->query('state', ''), + action: 'manifest', + ); + + abort_if($this->githubAppHasManifestCredentials($github_app), 403, 'GitHub App credentials are already configured.'); + + $api_url = data_get($github_app, 'api_url'); + $data = Http::withBody(null) + ->accept('application/vnd.github+json') + ->timeout(10) + ->connectTimeout(5) + ->post("$api_url/app-manifests/$code/conversions") + ->throw() + ->json(); + + $id = data_get($data, 'id'); + $slug = data_get($data, 'slug'); + $client_id = data_get($data, 'client_id'); + $client_secret = data_get($data, 'client_secret'); + $private_key = data_get($data, 'pem'); + $webhook_secret = data_get($data, 'webhook_secret'); + + abort_if(blank($id) || blank($slug) || blank($client_id) || blank($client_secret) || blank($private_key) || blank($webhook_secret), 422, 'GitHub App manifest conversion response is incomplete.'); + + $private_key = PrivateKey::create([ + 'name' => "github-app-{$slug}", + 'private_key' => $private_key, + 'team_id' => $github_app->team_id, + 'is_git_related' => true, + ]); + $github_app->name = $slug; + $github_app->app_id = $id; + $github_app->client_id = $client_id; + $github_app->client_secret = $client_secret; + $github_app->webhook_secret = $webhook_secret; + $github_app->private_key_id = $private_key->id; + $github_app->save(); + + return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); } public function install(Request $request) { - try { - $installation_id = $request->get('installation_id'); - $source = $request->get('source'); - $setup_action = $request->get('setup_action'); - $github_app = GithubApp::where('uuid', $source)->firstOrFail(); - if ($setup_action === 'install') { - $github_app->installation_id = $installation_id; - $github_app->save(); - } + $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]); - } catch (Exception $e) { - return handleError($e); + } + + $installation_id = (string) $request->query('installation_id', ''); + abort_unless(ctype_digit($installation_id), 422, 'Missing GitHub App installation id.'); + + abort_unless( + $this->githubInstallationBelongsToApp($github_app, $installation_id), + 403, + 'GitHub App installation could not be verified.' + ); + + $github_app->installation_id = $installation_id; + $github_app->save(); + + return redirect()->route('source.github.show', ['github_app_uuid' => $github_app->uuid]); + } + + /** + * Verify that the given installation id actually belongs to this GitHub App. + * + * 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. + */ + private function githubInstallationBelongsToApp(GithubApp $github_app, string $installation_id): bool + { + if (blank($github_app->app_id) || blank($github_app->privateKey?->private_key)) { + return false; + } + + try { + $jwt = generateGithubJwt($github_app); + $response = Http::withHeaders([ + 'Authorization' => "Bearer $jwt", + 'Accept' => 'application/vnd.github+json', + ]) + ->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; } } + + private function consumeGithubAppSetupState(Request $request, string $state, string $action): GithubApp + { + abort_if(blank($state), 404); + + $payload = Cache::pull($this->githubAppSetupStateCacheKey($state)); + abort_unless(is_array($payload), 404); + abort_unless(data_get($payload, 'action') === $action, 404); + + $team_id = $request->user()?->currentTeam()?->id; + abort_unless(! is_null($team_id) && (int) data_get($payload, 'team_id') === $team_id, 403); + + return GithubApp::whereKey(data_get($payload, 'github_app_id')) + ->where('team_id', data_get($payload, 'team_id')) + ->firstOrFail(); + } + + private function githubAppSetupStateCacheKey(string $state): string + { + return 'github-app-setup-state:'.hash('sha256', $state); + } + + private function githubAppHasManifestCredentials(GithubApp $github_app): bool + { + return filled($github_app->app_id) + || filled($github_app->client_id) + || filled($github_app->client_secret) + || filled($github_app->webhook_secret) + || filled($github_app->private_key_id); + } } diff --git a/app/Livewire/Source/Github/Change.php b/app/Livewire/Source/Github/Change.php index d6537069c..cc9ceea8a 100644 --- a/app/Livewire/Source/Github/Change.php +++ b/app/Livewire/Source/Github/Change.php @@ -7,7 +7,9 @@ use App\Models\GithubApp; use App\Models\PrivateKey; use App\Rules\SafeExternalUrl; use Illuminate\Foundation\Auth\Access\AuthorizesRequests; +use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Http; +use Illuminate\Support\Str; use Lcobucci\JWT\Configuration; use Lcobucci\JWT\Signer\Key\InMemory; use Lcobucci\JWT\Signer\Rsa\Sha256; @@ -72,6 +74,8 @@ class Change extends Component public $privateKeys; + public string $manifestState = ''; + protected function rules(): array { return [ @@ -147,6 +151,24 @@ class Change extends Component } } + private function githubAppSetupStateCacheKey(string $state): string + { + return 'github-app-setup-state:'.hash('sha256', $state); + } + + private function createGithubAppSetupState(string $action): string + { + $state = Str::random(64); + + Cache::put($this->githubAppSetupStateCacheKey($state), [ + 'action' => $action, + 'github_app_id' => $this->github_app->id, + 'team_id' => $this->github_app->team_id, + ], now()->addMinutes(60)); + + return $state; + } + public function checkPermissions() { try { @@ -211,6 +233,7 @@ class Change extends Component // Override name with kebab case for display $this->name = str($this->github_app->name)->kebab(); $this->fqdn = $settings->fqdn; + $this->manifestState = $this->createGithubAppSetupState('manifest'); if ($settings->public_ipv4) { $this->ipv4 = 'http://'.$settings->public_ipv4.':'.config('app.port'); diff --git a/resources/views/livewire/source/github/change.blade.php b/resources/views/livewire/source/github/change.blade.php index e47fb0ae0..623897de5 100644 --- a/resources/views/livewire/source/github/change.blade.php +++ b/resources/views/livewire/source/github/change.blade.php @@ -290,8 +290,8 @@ function createGithubApp(webhook_endpoint, preview_deployment_permissions, administration) { const { organization, - uuid, - html_url + html_url, + uuid } = @json($github_app); if (!webhook_endpoint) { alert('Please select a webhook endpoint.'); @@ -299,6 +299,7 @@ } let baseUrl = webhook_endpoint; const name = @js($name); + const manifestState = @js($manifestState); const isDev = @js(config('app.env')) === 'local'; const devWebhook = @js(config('constants.webhooks.dev_webhook')); @@ -340,7 +341,7 @@ }; const form = document.createElement('form'); form.setAttribute('method', 'post'); - form.setAttribute('action', `${html_url}/${path}?state=${uuid}`); + form.setAttribute('action', `${html_url}/${path}?state=${manifestState}`); const input = document.createElement('input'); input.setAttribute('id', 'manifest'); input.setAttribute('name', 'manifest'); diff --git a/routes/webhooks.php b/routes/webhooks.php index d8d8e094a..804fd7bcb 100644 --- a/routes/webhooks.php +++ b/routes/webhooks.php @@ -7,8 +7,11 @@ use App\Http\Controllers\Webhook\Gitlab; use App\Http\Controllers\Webhook\Stripe; use Illuminate\Support\Facades\Route; -Route::get('/source/github/redirect', [Github::class, 'redirect']); -Route::get('/source/github/install', [Github::class, 'install']); +Route::middleware(['web', 'auth', 'throttle:30,1'])->group(function () { + Route::get('/source/github/redirect', [Github::class, 'redirect']); + Route::get('/source/github/install', [Github::class, 'install']); +}); + Route::post('/source/github/events', [Github::class, 'normal']); Route::post('/source/github/events/manual', [Github::class, 'manual']); diff --git a/tests/Feature/Security/GithubAppSetupCallbackTest.php b/tests/Feature/Security/GithubAppSetupCallbackTest.php new file mode 100644 index 000000000..9c6893fd1 --- /dev/null +++ b/tests/Feature/Security/GithubAppSetupCallbackTest.php @@ -0,0 +1,257 @@ + InstanceSettings::query()->create(['id' => 0])); + + $this->team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + + $this->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, + ]); +}); + +function cacheGithubAppSetupState(string $state, string $action, GithubApp $githubApp): void +{ + Cache::put('github-app-setup-state:'.hash('sha256', $state), [ + 'action' => $action, + 'github_app_id' => $githubApp->id, + 'team_id' => $githubApp->team_id, + ], now()->addMinutes(15)); +} + +function authenticateGithubSetupCallbackTest(object $test): void +{ + $test->actingAs($test->user); + session(['currentTeam' => $test->team]); +} + +function fakeGithubManifestConversion(): void +{ + $key = openssl_pkey_new([ + 'private_key_bits' => 2048, + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ]); + openssl_pkey_export($key, $privateKey); + + Http::preventStrayRequests(); + Http::fake([ + 'https://api.github.com/app-manifests/*/conversions' => Http::response([ + 'id' => 987654, + 'slug' => 'attacker-controlled-app', + 'client_id' => 'new-client-id', + 'client_secret' => 'new-client-secret', + 'pem' => $privateKey, + 'webhook_secret' => 'new-webhook-secret', + ]), + ]); +} + +function configureGithubAppCredentials(GithubApp $githubApp): void +{ + $key = openssl_pkey_new([ + 'private_key_bits' => 2048, + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ]); + openssl_pkey_export($key, $privateKey); + + $privateKeyModel = PrivateKey::create([ + 'name' => 'github-app-test-key', + 'private_key' => $privateKey, + 'team_id' => $githubApp->team_id, + 'is_git_related' => true, + ]); + + $githubApp->forceFill([ + 'app_id' => 123456, + 'private_key_id' => $privateKeyModel->id, + ])->save(); +} + +function fakeGithubInstallationVerification(int $appId): void +{ + Http::preventStrayRequests(); + Http::fake([ + 'https://api.github.com/zen' => Http::response('Keep it logically awesome.', 200, [ + 'Date' => now()->toRfc7231String(), + ]), + 'https://api.github.com/app/installations/*' => Http::response([ + 'id' => 555, + 'app_id' => $appId, + ], 200), + ]); +} + +function fakeGithubInstallationVerificationFailure(): void +{ + Http::preventStrayRequests(); + Http::fake([ + 'https://api.github.com/zen' => Http::response('Keep it logically awesome.', 200, [ + 'Date' => now()->toRfc7231String(), + ]), + 'https://api.github.com/app/installations/*' => Http::response(['message' => 'Not Found'], 404), + ]); +} + +it('requires authentication before processing github app manifest callbacks', function () { + fakeGithubManifestConversion(); + cacheGithubAppSetupState('valid-state', 'manifest', $this->githubApp); + + $this->get('/webhooks/source/github/redirect?state=valid-state&code=attacker-code') + ->assertRedirect(); + + Http::assertNothingSent(); + + $this->githubApp->refresh(); + expect($this->githubApp->app_id)->toBeNull() + ->and($this->githubApp->client_id)->toBeNull() + ->and($this->githubApp->webhook_secret)->toBeNull(); +}); + +it('rejects github app manifest callbacks with invalid state without calling github', function () { + authenticateGithubSetupCallbackTest($this); + fakeGithubManifestConversion(); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/redirect?state='.$this->githubApp->uuid.'&code=attacker-code') + ->assertNotFound(); + + Http::assertNothingSent(); + + $this->githubApp->refresh(); + expect($this->githubApp->app_id)->toBeNull() + ->and($this->githubApp->client_id)->toBeNull() + ->and($this->githubApp->webhook_secret)->toBeNull(); +}); + +it('blocks rebinding an already configured github app through manifest callback', function () { + authenticateGithubSetupCallbackTest($this); + fakeGithubManifestConversion(); + + $this->githubApp->forceFill([ + 'app_id' => 123456, + 'client_id' => 'existing-client-id', + 'client_secret' => 'existing-client-secret', + 'webhook_secret' => 'existing-webhook-secret', + ])->save(); + + cacheGithubAppSetupState('valid-state', 'manifest', $this->githubApp); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/redirect?state=valid-state&code=attacker-code') + ->assertForbidden(); + + Http::assertNothingSent(); + + $this->githubApp->refresh(); + expect($this->githubApp->app_id)->toBe(123456) + ->and($this->githubApp->client_id)->toBe('existing-client-id') + ->and($this->githubApp->webhook_secret)->toBe('existing-webhook-secret'); +}); + +it('configures an unbound github app with a valid one-time manifest state', function () { + authenticateGithubSetupCallbackTest($this); + fakeGithubManifestConversion(); + cacheGithubAppSetupState('valid-state', 'manifest', $this->githubApp); + + $this->get('/webhooks/source/github/redirect?state=valid-state&code=real-code') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])); + + Http::assertSentCount(1); + + $this->githubApp->refresh(); + expect($this->githubApp->name)->toBe('attacker-controlled-app') + ->and($this->githubApp->app_id)->toBe(987654) + ->and($this->githubApp->client_id)->toBe('new-client-id') + ->and($this->githubApp->webhook_secret)->toBe('new-webhook-secret') + ->and($this->githubApp->private_key_id)->not->toBeNull(); +}); + +it('rejects replayed github app manifest states', function () { + authenticateGithubSetupCallbackTest($this); + fakeGithubManifestConversion(); + cacheGithubAppSetupState('valid-state', 'manifest', $this->githubApp); + + $this->get('/webhooks/source/github/redirect?state=valid-state&code=real-code') + ->assertRedirect(); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/redirect?state=valid-state&code=real-code') + ->assertNotFound(); + + Http::assertSentCount(1); +}); + +it('requires authentication before processing github app install callbacks', function () { + Http::preventStrayRequests(); + + $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=123456') + ->assertRedirect(); + + Http::assertNothingSent(); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBeNull(); +}); + +it('rejects github app install callbacks for an unknown github app', 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') + ->assertNotFound(); + + Http::assertNothingSent(); +}); + +it('rejects an installation id that github does not confirm belongs to the app', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + fakeGithubInstallationVerificationFailure(); + + $this->withHeader('Accept', 'application/json')->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=999999') + ->assertForbidden(); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBeNull(); +}); + +it('sets installation id when github confirms it belongs to the app', function () { + authenticateGithubSetupCallbackTest($this); + configureGithubAppCredentials($this->githubApp); + fakeGithubInstallationVerification($this->githubApp->app_id); + + $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&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('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); + + $this->get('/webhooks/source/github/install?source='.$this->githubApp->uuid.'&setup_action=install&installation_id=222222') + ->assertRedirect(route('source.github.show', ['github_app_uuid' => $this->githubApp->uuid])); + + $this->githubApp->refresh(); + expect($this->githubApp->installation_id)->toBe(222222); +});