fix: Remove server-side binary execution for security

Per maintainer feedback, we should not execute untrusted binaries on the
platform server as this could allow malicious code to infect the system.

Changes:
- Remove _verify_binary_commit function that ran binaries on the server
- Remove subprocess import (no longer needed)
- Update start_test to just log expected commit SHA
- Binary verification now happens only in the sandboxed GCP VMs via runCI scripts
- The runCI scripts (already added) log ccextractor --version for audit trail

The VM-based approach is secure because:
- VMs are isolated and disposable
- Each test runs in a fresh VM that gets destroyed after
- No way for malicious code to escape to the platform server

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>

Author: cfsmp3

mod_ci/controllers.py

@@ -6,12 +6,11 @@
 import os
 import re
 import shutil
-import subprocess
 import time
 import zipfile
 from collections import defaultdict
 from pathlib import Path
-from typing import Any, Dict, Optional, Tuple
+from typing import Any, Dict, Optional
 
 import googleapiclient.discovery
 import requests
@@ -342,61 +341,6 @@ def mark_test_failed(db, test, repository, message: str) -> None:
         log.error(f"Failed to mark test {test.id} as failed: {e}")
 
 
-def _verify_binary_commit(binary_path: str, expected_commit: str, log) -> Tuple[bool, str]:
-    """
-    Verify that the binary's embedded commit SHA matches the expected commit.
-
-    Runs the binary with --version and parses the output to extract the Git commit.
-    This ensures we're testing the correct binary and prevents race conditions.
-
-    :param binary_path: Path to the ccextractor binary
-    :type binary_path: str
-    :param expected_commit: The expected Git commit SHA
-    :type expected_commit: str
-    :param log: Logger instance
-    :return: Tuple of (success, message)
-    :rtype: tuple[bool, str]
-    """
-    try:
-        result = subprocess.run(
-            [binary_path, '--version'],
-            capture_output=True,
-            text=True,
-            timeout=30
-        )
-
-        # Parse output for Git commit line
-        # Example output: "    Git commit: 7a9acb7bd2cb1ae492b4a67d14c9e1687a0f607f"
-        for line in result.stdout.split('\n'):
-            if 'Git commit:' in line:
-                binary_commit = line.split('Git commit:')[1].strip()
-                if binary_commit == expected_commit:
-                    log.info(f"Binary commit verified: {binary_commit}")
-                    return True, f"Binary commit verified: {binary_commit}"
-                else:
-                    error_msg = (f"Binary commit mismatch! Expected {expected_commit}, "
-                                 f"but binary reports {binary_commit}")
-                    log.error(error_msg)
-                    return False, error_msg
-
-        error_msg = f"Could not find Git commit in binary --version output: {result.stdout[:500]}"
-        log.error(error_msg)
-        return False, error_msg
-
-    except subprocess.TimeoutExpired:
-        error_msg = "Binary --version timed out after 30 seconds"
-        log.error(error_msg)
-        return False, error_msg
-    except FileNotFoundError:
-        error_msg = f"Binary not found at {binary_path}"
-        log.error(error_msg)
-        return False, error_msg
-    except Exception as e:
-        error_msg = f"Error verifying binary commit: {e}"
-        log.error(error_msg)
-        return False, error_msg
-
-
 def start_test(compute, app, db, repository: Repository.Repository, test, bot_token) -> None:
     """
     Start a VM instance and run the tests.
@@ -563,25 +507,11 @@ def start_test(compute, app, db, repository: Repository.Repository, test, bot_to
             with zipfile.ZipFile(zip_path, 'r') as artifact_zip:
                 artifact_zip.extractall(base_folder)
 
-            # Verify the binary's embedded commit SHA matches expected commit
-            # This prevents race conditions where an old artifact might be used
-            if test.platform == TestPlatform.linux:
-                binary_path = os.path.join(base_folder, 'ccextractor')
-                # Make binary executable - this is safe because:
-                # 1. The binary comes from GitHub's official artifact storage
-                # 2. The path is in our controlled temp directory (base_folder)
-                # 3. We verify the binary's commit SHA immediately after
-                os.chmod(binary_path, 0o755)  # NOSONAR - Safe: trusted source, controlled path
-                verified, verify_msg = _verify_binary_commit(binary_path, test.commit, log)
-                if not verified:
-                    mark_test_failed(db, test, repository, verify_msg)
-                    return
-            else:
-                # Windows binaries can't be verified on Linux server
-                # The binary name is ccextractorwinfull.exe
-                # Log expected commit for manual verification from VM logs
-                log.info(f"Windows test {test.id}: expected binary commit is {test.commit}")
-                log.info(f"Binary version will be logged by runCI.bat on the Windows VM")
+            # Log expected commit - actual verification happens in the VM via runCI scripts
+            # The VM scripts log ccextractor --version output for audit trail
+            # This avoids running untrusted binaries on the platform server
+            log.info(f"Test {test.id} ({test.platform.value}): expected binary commit is {test.commit}")
+            log.info(f"Binary version will be logged by runCI script on the {test.platform.value} VM")
 
             artifact_saved = True
             break

tests/test_ci/test_controllers.py

@@ -6,9 +6,9 @@
 from flask import g
 
 from mod_auth.models import Role
-from mod_ci.controllers import (Workflow_builds, _verify_binary_commit,
-                                get_info_for_pr_comment, is_valid_commit_hash,
-                                progress_type_request, start_platforms)
+from mod_ci.controllers import (Workflow_builds, get_info_for_pr_comment,
+                                is_valid_commit_hash, progress_type_request,
+                                start_platforms)
 from mod_ci.models import BlockedUsers
 from mod_customized.models import CustomizedTest
 from mod_home.models import CCExtractorVersion, GeneralData
@@ -219,11 +219,8 @@ def test_cron_job_empty_token(self, mock_log):
     @mock.patch('mod_ci.controllers.g')
     @mock.patch('mod_ci.controllers.TestProgress')
     @mock.patch('mod_ci.controllers.GcpInstance')
-    @mock.patch('mod_ci.controllers._verify_binary_commit')
-    @mock.patch('os.chmod')
-    def test_start_test(self, mock_chmod, mock_verify_binary_commit, mock_gcp_instance,
-                        mock_test_progress, mock_g, mock_open_file,
-                        mock_create_instance, mock_wait_for_operation):
+    def test_start_test(self, mock_gcp_instance, mock_test_progress, mock_g,
+                        mock_open_file, mock_create_instance, mock_wait_for_operation):
         """Test start_test function."""
         import zipfile
 
@@ -269,9 +266,6 @@ def extractall(*args, **kwargs):
         mock_query = create_mock_db_query(mock_g)
         mock_query.c.got = MagicMock()
 
-        # Mock binary verification to succeed
-        mock_verify_binary_commit.return_value = (True, "Binary commit verified")
-
         # Test when gcp create instance fails
         mock_wait_for_operation.return_value = 'error occurred'
         start_test(mock.ANY, self.app, mock_g.db, repository, test, mock.ANY)
@@ -286,17 +280,6 @@ def extractall(*args, **kwargs):
         mock_create_instance.assert_called_once()
         mock_wait_for_operation.assert_called_once()
 
-        # Reset mocks for next test
-        mock_g.db.commit.reset_mock()
-        mock_create_instance.reset_mock()
-        mock_wait_for_operation.reset_mock()
-
-        # Test when binary commit verification fails
-        mock_verify_binary_commit.return_value = (False, "Binary commit mismatch!")
-        start_test(mock.ANY, self.app, mock_g.db, repository, test, mock.ANY)
-        # Should not proceed to create instance when verification fails
-        mock_create_instance.assert_not_called()
-
     @mock.patch('github.Github.get_repo')
     @mock.patch('mod_ci.controllers.start_test')
     @mock.patch('mod_ci.controllers.get_compute_service_object')
@@ -2381,101 +2364,3 @@ def generate_header(data, event, ci_key=None):
             'utf-8'), g.github['ci_key'] if ci_key is None else ci_key)
         headers = generate_git_api_header(event, sig)
         return headers
-
-
-class TestVerifyBinaryCommit(BaseTestCase):
-    """Test the _verify_binary_commit function."""
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_success(self, mock_run):
-        """Test successful commit verification."""
-        mock_log = MagicMock()
-        expected_commit = "abc123def456"
-
-        # Mock successful --version output
-        mock_run.return_value = MagicMock(
-            stdout="CCExtractor detailed version info\n"
-                   "    Version: 0.96\n"
-                   f"    Git commit: {expected_commit}\n"
-                   "    Compilation date: 2025-12-23\n"
-        )
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", expected_commit, mock_log)
-
-        self.assertTrue(success)
-        self.assertIn("verified", message)
-        mock_log.info.assert_called()
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_mismatch(self, mock_run):
-        """Test commit verification fails on mismatch."""
-        mock_log = MagicMock()
-        expected_commit = "abc123def456"
-        actual_commit = "different789"
-
-        mock_run.return_value = MagicMock(
-            stdout=f"    Git commit: {actual_commit}\n"
-        )
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", expected_commit, mock_log)
-
-        self.assertFalse(success)
-        self.assertIn("mismatch", message)
-        self.assertIn(expected_commit, message)
-        self.assertIn(actual_commit, message)
-        mock_log.error.assert_called()
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_no_git_line(self, mock_run):
-        """Test verification fails when Git commit line is missing."""
-        mock_log = MagicMock()
-
-        mock_run.return_value = MagicMock(
-            stdout="CCExtractor version info\n    Version: 0.96\n"
-        )
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", "abc123", mock_log)
-
-        self.assertFalse(success)
-        self.assertIn("Could not find Git commit", message)
-        mock_log.error.assert_called()
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_timeout(self, mock_run):
-        """Test verification handles timeout."""
-        import subprocess
-        mock_log = MagicMock()
-
-        mock_run.side_effect = subprocess.TimeoutExpired(cmd="test", timeout=30)
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", "abc123", mock_log)
-
-        self.assertFalse(success)
-        self.assertIn("timed out", message)
-        mock_log.error.assert_called()
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_file_not_found(self, mock_run):
-        """Test verification handles missing binary."""
-        mock_log = MagicMock()
-
-        mock_run.side_effect = FileNotFoundError()
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", "abc123", mock_log)
-
-        self.assertFalse(success)
-        self.assertIn("not found", message)
-        mock_log.error.assert_called()
-
-    @mock.patch('mod_ci.controllers.subprocess.run')
-    def test_verify_binary_commit_generic_exception(self, mock_run):
-        """Test verification handles unexpected exceptions."""
-        mock_log = MagicMock()
-
-        mock_run.side_effect = Exception("Unexpected error")
-
-        success, message = _verify_binary_commit("/path/to/ccextractor", "abc123", mock_log)
-
-        self.assertFalse(success)
-        self.assertIn("Error verifying", message)
-        mock_log.error.assert_called()