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) <noreply@anthropic.com>
2026-06-13 19:09:50 +00:00 · 2026-05-22 16:34:36 +02:00
parent fcd63f40eb
commit 5a7408a919
5 changed files with 413 additions and 45 deletions
@@ -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);
+    }
 }
@@ -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');
@@ -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');
@@ -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']);

@@ -0,0 +1,257 @@
+<?php
+
+use App\Models\GithubApp;
+use App\Models\InstanceSettings;
+use App\Models\PrivateKey;
+use App\Models\Team;
+use App\Models\User;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Http;
+
+uses(RefreshDatabase::class);
+
+beforeEach(function () {
+    InstanceSettings::unguarded(fn () => 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);
+});