From a565fc3b3623084c1efa0f2730eb9031c4f351c7 Mon Sep 17 00:00:00 2001
From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com>
Date: Fri, 27 Feb 2026 23:26:31 +0100
Subject: [PATCH] fix(rollback): escape commit SHA to prevent shell injection

Properly escape commit SHA using escapeshellarg() before passing it
to shell commands. Add comprehensive tests for git commit rollback
scenarios including shallow clone, fallback behavior, and HEAD handling.
---
 app/Models/Application.php                |  5 +-
 tests/Feature/ApplicationRollbackTest.php | 67 ++++++++++++++++++++---
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/app/Models/Application.php b/app/Models/Application.php
index 852b2ed2e..b4272b3c7 100644
--- a/app/Models/Application.php
+++ b/app/Models/Application.php
@@ -1096,12 +1096,13 @@ public function setGitImportSettings(string $deployment_uuid, string $git_clone_
         $commitToUse = $commit ?? $this->git_commit_sha;
 
         if ($commitToUse !== 'HEAD') {
+            $escapedCommit = escapeshellarg($commitToUse);
             // If shallow clone is enabled and we need a specific commit,
             // we need to fetch that specific commit with depth=1
             if ($isShallowCloneEnabled) {
-                $git_clone_command = "{$git_clone_command} && cd {$escapedBaseDir} && GIT_SSH_COMMAND=\"ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git fetch --depth=1 origin {$commitToUse} && git -c advice.detachedHead=false checkout {$commitToUse} >/dev/null 2>&1";
+                $git_clone_command = "{$git_clone_command} && cd {$escapedBaseDir} && GIT_SSH_COMMAND=\"ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git fetch --depth=1 origin {$escapedCommit} && git -c advice.detachedHead=false checkout {$escapedCommit} >/dev/null 2>&1";
             } else {
-                $git_clone_command = "{$git_clone_command} && cd {$escapedBaseDir} && GIT_SSH_COMMAND=\"ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git -c advice.detachedHead=false checkout {$commitToUse} >/dev/null 2>&1";
+                $git_clone_command = "{$git_clone_command} && cd {$escapedBaseDir} && GIT_SSH_COMMAND=\"ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null\" git -c advice.detachedHead=false checkout {$escapedCommit} >/dev/null 2>&1";
             }
         }
         if ($this->settings->is_git_submodules_enabled) {
diff --git a/tests/Feature/ApplicationRollbackTest.php b/tests/Feature/ApplicationRollbackTest.php
index dfb8da010..2b6141a92 100644
--- a/tests/Feature/ApplicationRollbackTest.php
+++ b/tests/Feature/ApplicationRollbackTest.php
@@ -11,7 +11,7 @@
 uses(RefreshDatabase::class);
 
 describe('Application Rollback', function () {
-    test('setGitImportSettings uses passed commit instead of application git_commit_sha', function () {
+    beforeEach(function () {
         $team = Team::factory()->create();
         $project = Project::create([
             'team_id' => $team->id,
@@ -25,31 +25,80 @@
         ]);
         $server = Server::factory()->create(['team_id' => $team->id]);
 
-        // Create application with git_commit_sha = 'HEAD' (default - use latest)
-        $application = Application::factory()->create([
+        $this->application = Application::factory()->create([
             'environment_id' => $environment->id,
             'destination_id' => $server->id,
             'git_commit_sha' => 'HEAD',
         ]);
+    });
 
-        // Create application settings
+    test('setGitImportSettings uses passed commit instead of application git_commit_sha', function () {
         ApplicationSetting::create([
-            'application_id' => $application->id,
+            'application_id' => $this->application->id,
             'is_git_shallow_clone_enabled' => false,
         ]);
 
-        // The rollback commit SHA we want to deploy
         $rollbackCommit = 'abc123def456';
 
-        // This should use the passed commit, not the application's git_commit_sha
-        $result = $application->setGitImportSettings(
+        $result = $this->application->setGitImportSettings(
             deployment_uuid: 'test-uuid',
             git_clone_command: 'git clone',
             public: true,
             commit: $rollbackCommit
         );
 
-        // Assert: The command should checkout the ROLLBACK commit
         expect($result)->toContain($rollbackCommit);
     });
+
+    test('setGitImportSettings with shallow clone fetches specific commit', function () {
+        ApplicationSetting::create([
+            'application_id' => $this->application->id,
+            'is_git_shallow_clone_enabled' => true,
+        ]);
+
+        $rollbackCommit = 'abc123def456';
+
+        $result = $this->application->setGitImportSettings(
+            deployment_uuid: 'test-uuid',
+            git_clone_command: 'git clone',
+            public: true,
+            commit: $rollbackCommit
+        );
+
+        expect($result)
+            ->toContain('git fetch --depth=1 origin')
+            ->toContain($rollbackCommit);
+    });
+
+    test('setGitImportSettings falls back to git_commit_sha when no commit passed', function () {
+        $this->application->update(['git_commit_sha' => 'def789abc012']);
+
+        ApplicationSetting::create([
+            'application_id' => $this->application->id,
+            'is_git_shallow_clone_enabled' => false,
+        ]);
+
+        $result = $this->application->setGitImportSettings(
+            deployment_uuid: 'test-uuid',
+            git_clone_command: 'git clone',
+            public: true,
+        );
+
+        expect($result)->toContain('def789abc012');
+    });
+
+    test('setGitImportSettings does not append checkout when commit is HEAD', function () {
+        ApplicationSetting::create([
+            'application_id' => $this->application->id,
+            'is_git_shallow_clone_enabled' => false,
+        ]);
+
+        $result = $this->application->setGitImportSettings(
+            deployment_uuid: 'test-uuid',
+            git_clone_command: 'git clone',
+            public: true,
+        );
+
+        expect($result)->not->toContain('advice.detachedHead=false checkout');
+    });
 });