Skip to content

Commit a165e03

Browse files
andrasbacsaiandrasbacsai
committed
fix(service-apps): harden updates and docker commands
Escape service application lifecycle command arguments for deploy, restart, stop, and log status checks. Validate API update payloads from JSON/form data before applying allowed fields, preserving explicit boolean and null values. Add coverage for service application API authorization, team isolation, validation, and command escaping.
1 parent 95a3c45 commit a165e03

9 files changed

Lines changed: 356 additions & 233 deletions

‎app/Actions/Service/DeployServiceApplication.php‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,35 +22,42 @@ public function handle(ServiceApplication $serviceApplication, bool $pullLatestI
2222
$service->isConfigurationChanged(save: true);
2323

2424
$workdir = $service->workdir();
25+
$composeFile = "{$workdir}/docker-compose.yml";
26+
$safeWorkdir = escapeshellarg($workdir);
27+
$safeComposeFile = escapeshellarg($composeFile);
28+
$safeProjectName = escapeshellarg($service->uuid);
29+
$safeComposeServiceName = escapeshellarg($composeServiceName);
30+
2531
$commands = collect([
26-
"echo 'Saved configuration files to {$workdir}.'",
27-
"touch {$workdir}/.env",
32+
'echo '.escapeshellarg("Saved configuration files to {$workdir}."),
33+
'touch '.escapeshellarg("{$workdir}/.env"),
2834
]);
2935

3036
if ($pullLatestImages) {
3137
$commands->push('echo Pulling image for service.');
32-
$commands->push("docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid} pull {$composeServiceName}");
38+
$commands->push("docker compose --project-directory {$safeWorkdir} -f {$safeComposeFile} --project-name {$safeProjectName} pull {$safeComposeServiceName}");
3339
}
3440

3541
if ($service->networks()->count() > 0) {
3642
$commands->push('echo Creating Docker network.');
37-
$commands->push("docker network inspect {$service->uuid} >/dev/null 2>&1 || docker network create --attachable {$service->uuid}");
43+
$commands->push("docker network inspect {$safeProjectName} >/dev/null 2>&1 || docker network create --attachable {$safeProjectName}");
3844
}
3945

40-
$upCommand = "docker compose --project-directory {$workdir} -f {$workdir}/docker-compose.yml --project-name {$service->uuid} up -d --no-deps";
46+
$upCommand = "docker compose --project-directory {$safeWorkdir} -f {$safeComposeFile} --project-name {$safeProjectName} up -d --no-deps";
4147
if ($forceRebuild) {
4248
$upCommand .= ' --build';
4349
}
44-
$upCommand .= " {$composeServiceName}";
50+
$upCommand .= " {$safeComposeServiceName}";
4551
$commands->push('echo Starting service container.');
4652
$commands->push($upCommand);
4753

48-
$commands->push("docker network connect {$service->uuid} coolify-proxy >/dev/null 2>&1 || true");
54+
$commands->push("docker network connect {$safeProjectName} coolify-proxy >/dev/null 2>&1 || true");
4955

5056
if (data_get($service, 'connect_to_docker_network')) {
51-
$compose = data_get($service, 'docker_compose', []);
52-
$network = $service->destination->network;
53-
$commands->push("docker network connect --alias {$composeServiceName}-{$service->uuid} {$network} {$composeServiceName}-{$service->uuid} >/dev/null 2>&1 || true");
57+
$network = escapeshellarg($service->destination->network);
58+
$containerName = escapeshellarg("{$composeServiceName}-{$service->uuid}");
59+
$networkAlias = escapeshellarg("{$composeServiceName}-{$service->uuid}");
60+
$commands->push("docker network connect --alias {$networkAlias} {$network} {$containerName} >/dev/null 2>&1 || true");
5461
}
5562

5663
return remote_process($commands->toArray(), $service->server, type_uuid: $service->uuid, callEventOnFinish: 'ServiceStatusChanged');

‎app/Actions/Service/RestartServiceApplication.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ public function handle(ServiceApplication $serviceApplication): void
1515
{
1616
$service = $serviceApplication->service;
1717
$server = $service->destination->server;
18-
$containerName = $serviceApplication->name.'-'.$service->uuid;
18+
$containerName = escapeshellarg($serviceApplication->name.'-'.$service->uuid);
1919

2020
instant_remote_process([
2121
"docker restart {$containerName}",

‎app/Actions/Service/StopServiceApplication.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ public function handle(ServiceApplication $serviceApplication): void
1515
{
1616
$service = $serviceApplication->service;
1717
$server = $service->destination->server;
18-
$containerName = $serviceApplication->name.'-'.$service->uuid;
18+
$containerName = escapeshellarg($serviceApplication->name.'-'.$service->uuid);
1919

2020
instant_remote_process([
2121
"docker stop {$containerName}",

‎app/Actions/Service/UpdateServiceApplicationFromApi.php‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@
99

1010
class UpdateServiceApplicationFromApi
1111
{
12-
public function execute(ServiceApplication $serviceApplication, Request $request, string $teamId): ?JsonResponse
12+
public function execute(ServiceApplication $serviceApplication, Request $request, string $teamId, array $payload): ?JsonResponse
1313
{
1414
$forceDomainOverride = $request->boolean('force_domain_override');
1515

16-
if ($request->has('url')) {
17-
$urlRaw = $request->input('url');
16+
if (array_key_exists('url', $payload)) {
17+
$urlRaw = $payload['url'];
1818
if ($urlRaw !== null && ! is_string($urlRaw)) {
1919
return response()->json([
2020
'message' => 'Validation failed.',
@@ -59,38 +59,38 @@ public function execute(ServiceApplication $serviceApplication, Request $request
5959
$serviceApplication->fqdn = $parsed['normalized'];
6060
}
6161

62-
if ($request->has('human_name')) {
63-
$serviceApplication->human_name = $request->input('human_name');
62+
if (array_key_exists('human_name', $payload)) {
63+
$serviceApplication->human_name = $payload['human_name'];
6464
}
6565

66-
if ($request->has('description')) {
67-
$serviceApplication->description = $request->input('description');
66+
if (array_key_exists('description', $payload)) {
67+
$serviceApplication->description = $payload['description'];
6868
}
6969

70-
if ($request->has('image')) {
71-
$serviceApplication->image = $request->input('image');
70+
if (array_key_exists('image', $payload)) {
71+
$serviceApplication->image = $payload['image'];
7272
}
7373

74-
if ($request->has('exclude_from_status')) {
75-
$serviceApplication->exclude_from_status = $request->boolean('exclude_from_status');
74+
if (array_key_exists('exclude_from_status', $payload)) {
75+
$serviceApplication->exclude_from_status = filter_var($payload['exclude_from_status'], FILTER_VALIDATE_BOOLEAN);
7676
}
7777

78-
if ($request->has('is_gzip_enabled')) {
79-
$serviceApplication->is_gzip_enabled = $request->boolean('is_gzip_enabled');
78+
if (array_key_exists('is_gzip_enabled', $payload)) {
79+
$serviceApplication->is_gzip_enabled = filter_var($payload['is_gzip_enabled'], FILTER_VALIDATE_BOOLEAN);
8080
}
8181

82-
if ($request->has('is_stripprefix_enabled')) {
83-
$serviceApplication->is_stripprefix_enabled = $request->boolean('is_stripprefix_enabled');
82+
if (array_key_exists('is_stripprefix_enabled', $payload)) {
83+
$serviceApplication->is_stripprefix_enabled = filter_var($payload['is_stripprefix_enabled'], FILTER_VALIDATE_BOOLEAN);
8484
}
8585

86-
if ($request->has('is_log_drain_enabled')) {
87-
$enabled = $request->boolean('is_log_drain_enabled');
86+
if (array_key_exists('is_log_drain_enabled', $payload)) {
87+
$enabled = filter_var($payload['is_log_drain_enabled'], FILTER_VALIDATE_BOOLEAN);
8888
$server = $serviceApplication->service->destination->server;
8989
if ($enabled && ! $server->isLogDrainEnabled()) {
9090
return response()->json([
9191
'message' => 'Validation failed.',
9292
'errors' => [
93-
'is_log_drain_enabled' => 'Log drain is not enabled on the server for this service.',
93+
'is_log_drain_enabled' => ['Log drain is not enabled on the server for this service.'],
9494
],
9595
], 422);
9696
}

‎app/Http/Controllers/Api/ServiceApplicationsController.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,11 @@ public function update(Request $request, UpdateServiceApplicationFromApi $update
306306

307307
$this->authorize('update', $serviceApplication);
308308

309+
$payload = $request->json()->all();
310+
if (empty($payload)) {
311+
$payload = $request->request->all();
312+
}
313+
309314
$allowedFields = [
310315
'url',
311316
'human_name',
@@ -328,9 +333,9 @@ public function update(Request $request, UpdateServiceApplicationFromApi $update
328333
'is_stripprefix_enabled' => 'sometimes|boolean',
329334
];
330335

331-
$validator = Validator::make($request->all(), $validationRules);
336+
$validator = Validator::make($payload, $validationRules);
332337

333-
$extraFields = array_diff(array_keys($request->all()), $allowedFields);
338+
$extraFields = array_diff(array_keys($payload), $allowedFields);
334339
if ($validator->fails() || ! empty($extraFields)) {
335340
$errors = $validator->errors();
336341
foreach ($extraFields as $field) {
@@ -343,7 +348,7 @@ public function update(Request $request, UpdateServiceApplicationFromApi $update
343348
], 422);
344349
}
345350

346-
$response = $updateServiceApplicationFromApi->execute($serviceApplication, $request, $teamId);
351+
$response = $updateServiceApplicationFromApi->execute($serviceApplication, $request, $teamId, $payload);
347352
if ($response instanceof JsonResponse) {
348353
return $response;
349354
}
@@ -450,8 +455,9 @@ public function logs_by_uuid(Request $request): JsonResponse
450455
}
451456

452457
$containerName = $serviceApplication->name.'-'.$serviceApplication->service->uuid;
458+
$safeContainerName = escapeshellarg($containerName);
453459

454-
$status = getContainerStatus($server, $containerName);
460+
$status = getContainerStatus($server, $safeContainerName);
455461
if ($status !== 'running') {
456462
return response()->json([
457463
'message' => 'Service application container is not running.',

0 commit comments

Comments
 (0)