diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index 8e9e277df7..669db96624 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -17,6 +17,8 @@ use App\Models\GithubApp; use App\Models\GithubRunnerConfig; use App\Models\GithubRunnerExecution; use App\Models\PrivateKey; +use App\Services\GithubRunner\GithubRunnerApi; +use App\Services\GithubRunner\GithubRunnerContainer; use Exception; use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\RedirectResponse; @@ -281,13 +283,16 @@ class Github extends Controller if ($action === 'queued') { $labels = array_values(array_filter((array) data_get($payload, 'workflow_job.labels', []), 'is_string')); - $matches = $githubApp->runnerConfigs() + $matchingConfigs = $githubApp->runnerConfigs() ->where('is_enabled', true) ->get() - ->contains(fn (GithubRunnerConfig $config) => $config->matchesLabels($labels)); - if (! $matches) { + ->filter(fn (GithubRunnerConfig $config) => $config->matchesLabels($labels)); + if ($matchingConfigs->isEmpty()) { return response('Nothing to do. No runner configuration matches the job labels.'); } + if (! $matchingConfigs->contains('allow_pull_requests', true) && $this->isPullRequestWorkflowJob($githubApp, $payload)) { + return response('Nothing to do. The runner configuration does not allow pull request jobs.'); + } $execution = GithubRunnerExecution::createOrFirst( ['github_app_id' => $githubApp->id, 'trigger_workflow_job_id' => $jobId], @@ -335,6 +340,29 @@ class Github extends Controller return response('Nothing to do.'); } + /** + * Whether a queued job belongs to a workflow run started by a pull request, so no runner is started + * for a job that the job-started hook would refuse. The `workflow_job` payload has no event, so the + * run is read from GitHub. When that fails, the job counts as no pull request job and the hook still + * refuses it in the runner. + */ + private function isPullRequestWorkflowJob(GithubApp $githubApp, Collection $payload): bool + { + $runId = (int) data_get($payload, 'workflow_job.run_id', 0); + $repository = (string) data_get($payload, 'repository.full_name', ''); + if ($runId <= 0 || ! str_contains($repository, '/')) { + return false; + } + + try { + $event = (new GithubRunnerApi($githubApp))->workflowRunEvent($repository, $runId); + } catch (\Throwable) { + return false; + } + + return in_array($event, GithubRunnerContainer::PULL_REQUEST_EVENTS, true); + } + public function normal(Request $request) { try { diff --git a/app/Services/GithubRunner/GithubRunnerApi.php b/app/Services/GithubRunner/GithubRunnerApi.php index 8e0ae46e76..dd65bbd998 100644 --- a/app/Services/GithubRunner/GithubRunnerApi.php +++ b/app/Services/GithubRunner/GithubRunnerApi.php @@ -16,9 +16,9 @@ class GithubRunnerApi public function __construct(private GithubApp $githubApp) {} /** - * Finds or creates the runner group of the App. Every repository of the organization, public ones too, - * can use it; runners refuse pull request jobs unless the runner config allows them. - * Groups created before public repositories were allowed are updated. + * Finds or creates the runner group of the App. A new group lets every repository of the organization, + * public ones too, use it; runners refuse pull request jobs unless the runner config allows them. + * Coolify does not change an existing group, so restrictions an organization admin set on GitHub stay. * All GitHub plans can create runner groups, so a failed creation is an error. Coolify never falls back * to the Default group, because that group can give the runners access to more repositories. * @@ -31,10 +31,6 @@ class GithubRunnerApi if ($this->githubApp->runner_group_id) { $response = $this->client()->get("/orgs/{$org}/actions/runner-groups/{$this->githubApp->runner_group_id}"); if ($response->successful()) { - if (! $response->json('default') && ($response->json('visibility') !== 'all' || $response->json('allows_public_repositories') !== true)) { - $this->allowAllRepositories((int) $this->githubApp->runner_group_id); - } - return ['id' => (int) $this->githubApp->runner_group_id, 'is_default' => (bool) $response->json('default')]; } if ($response->status() !== 404) { @@ -57,17 +53,6 @@ class GithubRunnerApi return $group; } - private function allowAllRepositories(int $runnerGroupId): void - { - $response = $this->client()->patch("/orgs/{$this->organization()}/actions/runner-groups/{$runnerGroupId}", [ - 'visibility' => 'all', - 'allows_public_repositories' => true, - ]); - if (! $response->successful()) { - throw new RuntimeException('Could not update the runner group: '.$this->errorMessage($response)); - } - } - /** * @param array $labels * @return array{runner_id: int, encoded_jit_config: string} @@ -91,6 +76,22 @@ class GithubRunnerApi ]; } + /** + * The event that started a workflow run, for example "push" or "pull_request". The `workflow_job` + * webhook does not contain it. A short timeout keeps the webhook response fast. + */ + public function workflowRunEvent(string $repositoryFullName, int $runId): string + { + $repository = implode('/', array_map('rawurlencode', explode('/', $repositoryFullName, 2))); + $response = $this->client()->timeout(5)->get("/repos/{$repository}/actions/runs/{$runId}"); + + if (! $response->successful() || ! is_string($response->json('event'))) { + throw new RuntimeException('Could not read the workflow run: '.$this->errorMessage($response)); + } + + return $response->json('event'); + } + /** * Removes a runner from GitHub. GitHub removes JIT runners itself after their job, so 404 is not an error. */ diff --git a/app/Services/GithubRunner/GithubRunnerContainer.php b/app/Services/GithubRunner/GithubRunnerContainer.php index 82fa0433c8..d7815f5769 100644 --- a/app/Services/GithubRunner/GithubRunnerContainer.php +++ b/app/Services/GithubRunner/GithubRunnerContainer.php @@ -30,6 +30,9 @@ class GithubRunnerContainer private const HOOK_PATH = '/home/runner/coolify-job-started.sh'; + /** Workflow run events that a runner refuses unless the config allows pull request jobs. */ + public const PULL_REQUEST_EVENTS = ['pull_request', 'pull_request_target', 'pull_request_review', 'pull_request_review_comment']; + public function __construct(private GithubRunnerExecution $execution, private GithubRunnerConfig $config) {} public static function nameFor(string $executionUuid): string @@ -153,10 +156,12 @@ class GithubRunnerContainer */ public static function pullRequestHookScript(): string { - return <<<'BASH' + $events = implode('|', self::PULL_REQUEST_EVENTS); + + return << Http::response('ok', 200, ['Date' => now()->toRfc7231String()]), + "https://api.github.com/app/installations/{$githubApp->installation_id}/access_tokens" => Http::response(['token' => 'installation-token'], 201), + 'https://api.github.com/repos/acme/api/actions/runs/321' => $runResponse, + ]); +} + describe('workflow_job webhook', function () { beforeEach(function () { Queue::fake(); @@ -165,6 +174,43 @@ describe('workflow_job webhook', function () { 'github hosted' => [['ubuntu-latest']], ]); + it('does not provision a runner for a pull request job when no matching configuration allows them', function (string $event) { + Http::preventStrayRequests(); + fakeWorkflowRunApi($this->githubApp, Http::response(['id' => 321, 'event' => $event])); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'queued', ['run_id' => 321]) + ->assertOk() + ->assertSee('does not allow pull request jobs'); + + expect(GithubRunnerExecution::count())->toBe(0); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class); + })->with(['pull_request', 'pull_request_target']); + + it('provisions a runner for a job that does not come from a pull request', function (mixed $runResponse) { + Http::preventStrayRequests(); + fakeWorkflowRunApi($this->githubApp, $runResponse); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'queued', ['run_id' => 321])->assertOk(); + + expect(GithubRunnerExecution::sole()->status)->toBe(GithubRunnerStatus::Queued); + Queue::assertPushed(ProvisionGithubRunnerJob::class, 1); + })->with([ + 'push' => fn () => Http::response(['id' => 321, 'event' => 'push']), + 'workflow run lookup fails' => fn () => Http::response(['message' => 'Server Error'], 500), + ]); + + it('provisions a runner for a pull request job without a lookup when a matching configuration allows them', function () { + Http::preventStrayRequests(); + Http::fake(); + GithubRunnerConfig::query()->update(['allow_pull_requests' => true]); + + sendWorkflowJobWebhook($this, $this->githubApp, 'runner-secret', 'queued', ['run_id' => 321])->assertOk(); + + expect(GithubRunnerExecution::count())->toBe(1); + Queue::assertPushed(ProvisionGithubRunnerJob::class, 1); + Http::assertNothingSent(); + }); + it('rejects an invalid signature', function () { sendWorkflowJobWebhook($this, $this->githubApp, 'wrong-secret', 'queued', [])->assertSee('Invalid signature'); diff --git a/tests/Feature/GithubRunnerSettingsTest.php b/tests/Feature/GithubRunnerSettingsTest.php index df4619b991..65052a0fcb 100644 --- a/tests/Feature/GithubRunnerSettingsTest.php +++ b/tests/Feature/GithubRunnerSettingsTest.php @@ -133,14 +133,12 @@ describe('runner settings page', function () { expect($config->allow_pull_requests)->toBeFalse(); }); - it('opens an existing private runner group to public repositories', function () { + it('keeps the restrictions an organization admin set on an existing runner group', function () { $this->githubApp->update(['runner_group_id' => 77]); Http::fake([ 'https://api.github.com/zen' => Http::response('ok', 200, ['Date' => now()->toRfc7231String()]), "https://api.github.com/app/installations/{$this->githubApp->installation_id}/access_tokens" => Http::response(['token' => 'installation-token'], 201), - 'https://api.github.com/orgs/acme/actions/runner-groups/77' => fn ($request) => $request->method() === 'PATCH' - ? Http::response(['id' => 77, 'default' => false, 'visibility' => 'all', 'allows_public_repositories' => true]) - : Http::response(['id' => 77, 'default' => false, 'visibility' => 'private', 'allows_public_repositories' => false]), + 'https://api.github.com/orgs/acme/actions/runner-groups/77' => Http::response(['id' => 77, 'default' => false, 'visibility' => 'selected', 'allows_public_repositories' => false]), ]); Livewire::actingAs($this->owner) @@ -150,10 +148,10 @@ describe('runner settings page', function () { ->assertHasNoErrors() ->assertDispatched('success'); - Http::assertSent(fn ($request) => $request->method() === 'PATCH' - && str_ends_with($request->url(), '/orgs/acme/actions/runner-groups/77') - && $request['allows_public_repositories'] === true - && $request['visibility'] === 'all'); + expect($this->githubApp->fresh()->runner_group_id)->toBe(77) + ->and(GithubRunnerConfig::count())->toBe(1); + Http::assertNotSent(fn ($request) => in_array($request->method(), ['PATCH', 'POST'], true) + && str_contains($request->url(), '/runner-groups')); }); it('saves whether runners take pull request jobs', function () {