Improve GitHub App setup flow (#10524)

app/Http/Controllers/Webhook/Github.php

@@ -11,6 +11,8 @@ use App\Models\Application;
 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 @@ class Github extends Controller
 
     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 @@ class Github extends Controller
         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 @@ class Github extends Controller
 
     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 @@ class Github extends Controller
             ->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);

app/Livewire/Source/Github/Change.php

@@ -210,6 +210,9 @@ class Change extends Component
 
             GithubAppPermissionJob::dispatchSync($this->github_app);
             $this->github_app->refresh()->makeVisible('client_secret')->makeVisible('webhook_secret');
+            $this->syncData(false);
+            $this->name = str($this->github_app->name)->kebab();
+
             $this->dispatch('success', 'Github App permissions updated.');
         } catch (\Throwable $e) {
             // Provide better error message for unsupported key formats

bootstrap/helpers/github.php

@@ -4,6 +4,7 @@ use App\Models\GithubApp;
 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:<br>'.
             '- System time: '.$serverTime->format('Y-m-d H:i:s').' UTC<br>'.
             '- GitHub time: '.$githubTime->format('Y-m-d H:i:s').' UTC<br>'.
@@ -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:<br>'.
             "Error: {$errorMessage}<br>".
             'Rate Limit Status:<br>'.
@@ -116,13 +117,19 @@ 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';
+    $name = str(Str::kebab($source->name));
+    $installation_path = $source->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' => $source->id,
+        'team_id' => $source->team_id,
+    ], now()->addMinutes(60));
+
+    return "$source->html_url/$installation_path/$name/installations/new?".http_build_query(['state' => $state]);
 }
 
 function getPermissionsPath(GithubApp $source)

resources/views/livewire/source/github/change.blade.php

@@ -351,9 +351,8 @@
                 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 @@
                         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

tests/Feature/GithubSourceChangeTest.php

@@ -7,6 +7,8 @@ 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;
 use Livewire\Livewire;
 
 uses(RefreshDatabase::class);
@@ -84,6 +86,67 @@ describe('GitHub Source Change Component', function () {
             ->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('installation path is generated from the provided github app instance', function () {
+        $githubApp = new GithubApp;
+        $githubApp->forceFill([
+            'id' => 123,
+            'name' => 'Provided GitHub App',
+            'html_url' => 'https://github.example.com',
+            'team_id' => 456,
+        ]);
+
+        $installationUrl = getInstallationPath($githubApp);
+        parse_str(parse_url($installationUrl, PHP_URL_QUERY), $query);
+        $installState = $query['state'] ?? null;
+
+        expect($installationUrl)->toStartWith('https://github.example.com/github-apps/provided-git-hub-app/installations/new?')
+            ->and($installState)->not->toBeEmpty()
+            ->and(Cache::get('github-app-setup-state:'.hash('sha256', $installState)))
+            ->toMatchArray([
+                'action' => 'install',
+                'github_app_id' => 123,
+                'team_id' => 456,
+            ]);
+    });
+
     test('defaults webhook endpoint to app url when it is the first available endpoint', function () {
         config(['app.url' => 'http://localhost:8000']);
 
@@ -305,4 +368,65 @@ describe('GitHub Source Change Component', function () {
                 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('name', 'test-git-hub-app')
+            ->assertSet('contents', null)
+            ->assertSet('metadata', null)
+            ->assertSet('pullRequests', null)
+            ->call('checkPermissions')
+            ->assertDispatched('success')
+            ->assertSet('name', 'test-git-hub-app')
+            ->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');
+    });
 });

tests/Feature/Security/GithubAppSetupCallbackTest.php

@@ -199,8 +199,9 @@ it('rejects replayed github app manifest states', function () {
 
 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 @@ it('requires authentication before processing github app install callbacks', fun
     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 @@ it('sets installation id when github confirms it belongs to the app', function (
     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();