mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(github): deploy same-repo PR previews regardless of author association
GitHub reports team-granted and private org members as CONTRIBUTOR or NONE, so their same-repository PRs were silently skipped when public PR deployments were off. A same-repository branch already requires push access, so gate only on fork status, treat an unknown head repository as a fork, and log skipped fork PRs. Closes #9271
This commit is contained in:
@@ -71,7 +71,6 @@ class Github extends Controller
|
||||
'before_sha' => $before_sha,
|
||||
'after_sha' => $after_sha,
|
||||
'commit_sha' => $commit_sha,
|
||||
'author_association' => $author_association,
|
||||
'is_fork_pull_request' => $is_fork_pull_request,
|
||||
] = $this->readPullRequestPayload($payload);
|
||||
$full_name = data_get($payload, 'repository.full_name');
|
||||
@@ -239,7 +238,6 @@ class Github extends Controller
|
||||
beforeSha: $before_sha,
|
||||
afterSha: $after_sha,
|
||||
commitSha: $commit_sha,
|
||||
authorAssociation: $author_association,
|
||||
fullName: $full_name,
|
||||
isForkPullRequest: $is_fork_pull_request,
|
||||
);
|
||||
@@ -459,7 +457,6 @@ class Github extends Controller
|
||||
'before_sha' => $before_sha,
|
||||
'after_sha' => $after_sha,
|
||||
'commit_sha' => $commit_sha,
|
||||
'author_association' => $author_association,
|
||||
'is_fork_pull_request' => $is_fork_pull_request,
|
||||
] = $this->readPullRequestPayload($payload);
|
||||
$full_name = $this->manualWebhookRepositoryFullName(data_get($payload, 'repository.full_name'));
|
||||
@@ -601,7 +598,6 @@ class Github extends Controller
|
||||
beforeSha: $before_sha,
|
||||
afterSha: $after_sha,
|
||||
commitSha: $commit_sha,
|
||||
authorAssociation: $author_association,
|
||||
fullName: $full_name,
|
||||
isForkPullRequest: $is_fork_pull_request,
|
||||
);
|
||||
@@ -655,7 +651,7 @@ class Github extends Controller
|
||||
/**
|
||||
* Read and validate the pull_request payload fields that the handlers use.
|
||||
*
|
||||
* @return array{action: string, pull_request_id: int, pull_request_html_url: string, pull_request_title: ?string, branch: ?string, base_branch: ?string, before_sha: ?string, after_sha: ?string, commit_sha: string, author_association: ?string, is_fork_pull_request: bool}
|
||||
* @return array{action: string, pull_request_id: int, pull_request_html_url: string, pull_request_title: ?string, branch: ?string, base_branch: ?string, before_sha: ?string, after_sha: ?string, commit_sha: string, is_fork_pull_request: bool}
|
||||
*
|
||||
* @throws InvalidWebhookPayloadException When a field has a wrong type or format.
|
||||
*/
|
||||
@@ -673,7 +669,6 @@ class Github extends Controller
|
||||
'before_sha' => $this->webhookCommitSha($payload, 'before'),
|
||||
'after_sha' => $this->webhookCommitSha($payload, 'after') ?? $head_sha,
|
||||
'commit_sha' => $head_sha ?? 'HEAD',
|
||||
'author_association' => $this->webhookPayloadString($payload, 'pull_request.author_association'),
|
||||
'is_fork_pull_request' => $this->isForkPullRequest($payload),
|
||||
];
|
||||
}
|
||||
@@ -681,13 +676,15 @@ class Github extends Controller
|
||||
/**
|
||||
* Determine whether a pull_request webhook payload originates from a fork.
|
||||
*
|
||||
* GitHub's `author_association` is not a reliable trust signal (it grants
|
||||
* CONTRIBUTOR to anyone who has merely opened an issue/PR before), so fork
|
||||
* detection is gated on whether the PR crosses repository boundaries.
|
||||
* A same-repository PR branch can only be pushed by users with write access,
|
||||
* so this is the trust signal for preview deployments. GitHub's
|
||||
* `author_association` is not reliable for this: users with access through a
|
||||
* team or a private organization membership arrive as CONTRIBUTOR or NONE.
|
||||
*
|
||||
* The repository id comparison is the canonical signal; the `head.repo.fork`
|
||||
* flag and a case-insensitive full_name comparison are fallbacks for payloads
|
||||
* where the ids are unavailable (e.g. a deleted head repository).
|
||||
* where the ids are unavailable. A PR whose head repository is unknown (e.g.
|
||||
* a deleted fork) is treated as a fork.
|
||||
*
|
||||
* @throws InvalidWebhookPayloadException When a repository id is not a positive integer.
|
||||
*/
|
||||
@@ -711,7 +708,7 @@ class Github extends Controller
|
||||
return Str::lower($headRepoFullName) !== Str::lower($baseRepoFullName);
|
||||
}
|
||||
|
||||
return false;
|
||||
return true;
|
||||
}
|
||||
|
||||
public function redirect(Request $request)
|
||||
|
||||
@@ -38,7 +38,6 @@ class ProcessGithubPullRequestWebhook implements ShouldBeEncrypted, ShouldQueue
|
||||
public ?string $beforeSha,
|
||||
public ?string $afterSha,
|
||||
public string $commitSha,
|
||||
public ?string $authorAssociation,
|
||||
public string $fullName,
|
||||
public bool $isForkPullRequest = false,
|
||||
) {
|
||||
@@ -110,22 +109,16 @@ class ProcessGithubPullRequestWebhook implements ShouldBeEncrypted, ShouldQueue
|
||||
return;
|
||||
}
|
||||
|
||||
// Check if PR deployments from public contributors are restricted
|
||||
if (! $application->settings->is_pr_deployments_public_enabled) {
|
||||
// Fork PRs carry untrusted code from a repository outside our control.
|
||||
// GitHub's author_association cannot be trusted to gate these (it grants
|
||||
// CONTRIBUTOR to anyone who has merely opened an issue/PR before), so fork
|
||||
// PRs are never deployed automatically when public previews are off.
|
||||
if ($this->isForkPullRequest) {
|
||||
return;
|
||||
}
|
||||
// Fork PRs carry untrusted code, so they deploy only when public previews are on.
|
||||
// Same-repository branches can only be pushed by users with write access.
|
||||
if ($this->isForkPullRequest && ! $application->settings->is_pr_deployments_public_enabled) {
|
||||
Log::info('Skipped GitHub pull request preview deployment from a fork because public PR deployments are disabled.', [
|
||||
'application_uuid' => $application->uuid,
|
||||
'pull_request_id' => $this->pullRequestId,
|
||||
'action' => $this->action,
|
||||
]);
|
||||
|
||||
// Same-repo (non-fork) branch PRs require push access to the base repo,
|
||||
// so only trusted associations are allowed to trigger a deployment.
|
||||
$trustedAssociations = ['OWNER', 'MEMBER', 'COLLABORATOR'];
|
||||
if (! in_array($this->authorAssociation, $trustedAssociations)) {
|
||||
return;
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// Get changed files for watch path filtering (null means the full list is unknown)
|
||||
|
||||
@@ -86,7 +86,7 @@
|
||||
|
||||
<div class="w-full">
|
||||
<x-forms.listbox id="isPrDeploymentsPublicEnabled" label="PR deployment access" onChange="savePreviewSettings"
|
||||
helper="When public, anyone can trigger PR deployments. Otherwise fork PRs are blocked and only repository owners, members, and collaborators can trigger them."
|
||||
helper="When public, anyone can trigger PR deployments. Otherwise only PRs from branches in the same repository are deployed and fork PRs are blocked."
|
||||
:options="[
|
||||
['value' => false, 'label' => 'Repository members only'],
|
||||
['value' => true, 'label' => 'Public (fork PRs allowed)'],
|
||||
|
||||
@@ -185,7 +185,6 @@ test('webhook jobs run on the webhooks queue on cloud and on high on self-hosted
|
||||
beforeSha: null,
|
||||
afterSha: null,
|
||||
commitSha: 'abc123',
|
||||
authorAssociation: null,
|
||||
fullName: 'coollabsio/coolify',
|
||||
);
|
||||
|
||||
|
||||
@@ -54,7 +54,7 @@ it('cleans up a closed pull request preview when pull request comment cleanup fa
|
||||
'status' => 'success',
|
||||
]);
|
||||
|
||||
$job = new class(applicationId: $this->application->id, githubAppId: null, action: 'closed', pullRequestId: 42, pullRequestHtmlUrl: 'https://github.com/example/repo/pull/42', pullRequestTitle: null, beforeSha: null, afterSha: null, commitSha: 'HEAD', authorAssociation: 'OWNER', fullName: 'example/repo') extends ProcessGithubPullRequestWebhook
|
||||
$job = new class(applicationId: $this->application->id, githubAppId: null, action: 'closed', pullRequestId: 42, pullRequestHtmlUrl: 'https://github.com/example/repo/pull/42', pullRequestTitle: null, beforeSha: null, afterSha: null, commitSha: 'HEAD', fullName: 'example/repo') extends ProcessGithubPullRequestWebhook
|
||||
{
|
||||
protected function dispatchPullRequestClosedUpdate(Application $application, ApplicationPreview $preview): void
|
||||
{
|
||||
@@ -98,7 +98,6 @@ it('skips a synchronized GitHub pull request preview when the head commit messag
|
||||
beforeSha: 'before-sha',
|
||||
afterSha: 'after-sha',
|
||||
commitSha: 'after-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -148,7 +147,6 @@ it('deploys a synchronized GitHub pull request preview when the head commit mess
|
||||
beforeSha: 'before-sha',
|
||||
afterSha: 'after-sha',
|
||||
commitSha: 'after-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -195,7 +193,6 @@ it('skips a GitHub pull request preview when the opened or reopened head commit
|
||||
beforeSha: null,
|
||||
afterSha: null,
|
||||
commitSha: 'head-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -246,7 +243,6 @@ it('deploys an opened GitHub pull request preview when a watched file is beyond
|
||||
beforeSha: null,
|
||||
afterSha: null,
|
||||
commitSha: 'head-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -291,7 +287,6 @@ it('deploys a GitHub pull request preview when the full list of changed files is
|
||||
beforeSha: 'before-sha',
|
||||
afterSha: 'after-sha',
|
||||
commitSha: 'after-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -348,7 +343,6 @@ it('skips an opened GitHub pull request preview when no changed file matches the
|
||||
beforeSha: null,
|
||||
afterSha: null,
|
||||
commitSha: 'head-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -414,7 +408,6 @@ it('uses one GitHub App installation token for all GitHub API calls of a pull re
|
||||
beforeSha: null,
|
||||
afterSha: null,
|
||||
commitSha: 'head-sha',
|
||||
authorAssociation: 'OWNER',
|
||||
fullName: 'example/repo',
|
||||
);
|
||||
|
||||
@@ -429,3 +422,50 @@ it('uses one GitHub App installation token for all GitHub API calls of a pull re
|
||||
->and($repositoryRequests)->toHaveCount(3)
|
||||
->and($repositoryRequests->every(fn (array $pair): bool => $pair[0]->hasHeader('Authorization', 'Bearer fake-installation-token')))->toBeTrue();
|
||||
});
|
||||
|
||||
it('deploys an opened GitHub pull request preview by fork status, not by author association', function (bool $isForkPullRequest, bool $isPublicEnabled, bool $deploys) {
|
||||
Queue::fake();
|
||||
|
||||
$this->application->settings->update([
|
||||
'is_preview_deployments_enabled' => true,
|
||||
'is_pr_deployments_public_enabled' => $isPublicEnabled,
|
||||
]);
|
||||
|
||||
Http::fake([
|
||||
'https://api.github.com/repos/example/repo/commits/head-sha' => Http::response([
|
||||
'commit' => ['message' => 'feat: add app'],
|
||||
]),
|
||||
'https://api.github.com/repos/example/repo/pulls/42/files*' => Http::response([
|
||||
['filename' => 'README.md'],
|
||||
]),
|
||||
]);
|
||||
|
||||
$job = new ProcessGithubPullRequestWebhook(
|
||||
applicationId: $this->application->id,
|
||||
githubAppId: GithubApp::create([
|
||||
'name' => 'Public GitHub',
|
||||
'api_url' => 'https://api.github.com',
|
||||
'html_url' => 'https://github.com',
|
||||
'is_public' => true,
|
||||
'team_id' => $this->team->id,
|
||||
])->id,
|
||||
action: 'opened',
|
||||
pullRequestId: 42,
|
||||
pullRequestHtmlUrl: 'https://github.com/example/repo/pull/42',
|
||||
pullRequestTitle: 'Add feature',
|
||||
beforeSha: null,
|
||||
afterSha: 'head-sha',
|
||||
commitSha: 'head-sha',
|
||||
fullName: 'example/repo',
|
||||
isForkPullRequest: $isForkPullRequest,
|
||||
);
|
||||
|
||||
$job->handle();
|
||||
|
||||
expect(ApplicationPreview::where('application_id', $this->application->id)->where('pull_request_id', 42)->exists())->toBe($deploys)
|
||||
->and(ApplicationDeploymentQueue::where('application_id', $this->application->id)->where('pull_request_id', 42)->exists())->toBe($deploys);
|
||||
})->with([
|
||||
'same-repository branch with public deployments off' => [false, false, true],
|
||||
'fork with public deployments off' => [true, false, false],
|
||||
'fork with public deployments on' => [true, true, true],
|
||||
]);
|
||||
|
||||
@@ -285,7 +285,6 @@ function webhookTypesFields(): array
|
||||
['number', 'id', null],
|
||||
['pull_request.html_url', 'url', null],
|
||||
['pull_request.title', 'string', null],
|
||||
['pull_request.author_association', 'string', null],
|
||||
['pull_request.head.ref', 'branch', 'No branch'],
|
||||
['pull_request.base.ref', 'branch', 'No branch'],
|
||||
['pull_request.head.sha', 'sha', null],
|
||||
@@ -311,7 +310,6 @@ function webhookTypesFields(): array
|
||||
['number', 'id', null],
|
||||
['pull_request.html_url', 'url', null],
|
||||
['pull_request.title', 'string', null],
|
||||
['pull_request.author_association', 'string', null],
|
||||
['pull_request.head.ref', 'branch', 'No id or branch'],
|
||||
['pull_request.base.ref', 'branch', 'No id or branch'],
|
||||
['pull_request.head.sha', 'sha', null],
|
||||
@@ -473,11 +471,20 @@ describe('Webhook payload value types', function () {
|
||||
&& $job->pullRequestHtmlUrl === 'https://github.com/test-org/test-repo/pull/42'
|
||||
&& $job->pullRequestTitle === 'Add feature'
|
||||
&& $job->commitSha === webhookTypesSha()
|
||||
&& $job->authorAssociation === 'OWNER'
|
||||
&& $job->fullName === 'test-org/test-repo'
|
||||
&& $job->isForkPullRequest === false);
|
||||
})->with(['github manual', 'github app'])->with(['open', 'closed']);
|
||||
|
||||
test('a github pull request is queued as a fork when its head repository is unknown', function (string $handler) {
|
||||
$application = webhookTypesApplication($handler);
|
||||
$payload = webhookTypesPayload($handler, 'pr');
|
||||
unset($payload['pull_request']['head']['repo']);
|
||||
|
||||
webhookTypesSend($this, $handler, 'pr', 'open', $application, $payload)->assertOk();
|
||||
|
||||
Queue::assertPushed(ProcessGithubPullRequestWebhook::class, fn (ProcessGithubPullRequestWebhook $job): bool => $job->isForkPullRequest === true);
|
||||
})->with(['github manual', 'github app']);
|
||||
|
||||
test('a github pull request with a numeric string number and sync shas is queued', function (string $handler) {
|
||||
$application = webhookTypesApplication($handler);
|
||||
$payload = webhookTypesPayload($handler, 'pr');
|
||||
|
||||
Reference in New Issue
Block a user