Fix 10 code review findings: security, correctness, and SQLite compat

- system.php: fix null dereference on fetchOne (TypeError on null['value'])
- system.php: validate update_channel to ['stable','beta'] to prevent shell injection
- system.php: escapeshellarg remoteBranch in git log/show calls (was RCE vector)
- system.php: fix backup path — rsync contents, not directory, so restore is symmetric
- system.php: syntax check only changed files (git diff) not all 300+ panel files
- system.php: copy VERSION to $webRoot/VERSION not $webRoot/../VERSION (wrong path)
- system.php: fix 3× ON DUPLICATE KEY UPDATE → SQLite ON CONFLICT syntax
- deploy-runner.sh: hoist DB_PATH/CHANNEL above while loop
- deploy-runner.sh: sanitize NEW_VERSION and commit hashes before SQL interpolation
- deploy-runner.sh: parse queued branch (4th field) from webhook queue entry
- webhook.php: remove dead $branch config variable
- webhook.php: include pushed branch in queue entry to eliminate TOCTOU race

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

deploy/deploy-runner.sh

@@ -15,15 +15,21 @@ log() { echo "[$(date '+%Y-%m-%d %H:%M:%S')] $*" >> "$LOG"; }
 exec 9>"$LOCK"
 flock -n 9 || { log "Deploy already running, skipping"; exit 0; }
 
-while IFS='|' read -r REPO_PATH WEB_ROOT COMMIT; do
+# Read DB path and channel once before processing the queue
-  [[ -z "$REPO_PATH" ]] && continue
+DB_PATH=$(python3 -c "import configparser; c=configparser.ConfigParser(); c.read('/etc/novacpx/config.ini'); print(c.get('database','path',fallback='/var/lib/novacpx/panel.db'))" 2>/dev/null || echo "/var/lib/novacpx/panel.db")
-  log "--- Deploying commit $COMMIT ---"
+CHANNEL=$(sqlite3 "$DB_PATH" "SELECT value FROM settings WHERE key='update_channel'" 2>/dev/null || echo "stable")
+[[ "$CHANNEL" != "beta" ]] && CHANNEL="stable"
 
-  # Read update channel from DB to know which branch to pull
+while IFS='|' read -r REPO_PATH WEB_ROOT COMMIT QUEUED_BRANCH; do
-  DB_PATH=$(python3 -c "import configparser; c=configparser.ConfigParser(); c.read('/etc/novacpx/config.ini'); print(c.get('database','path',fallback='/var/lib/novacpx/panel.db'))" 2>/dev/null || echo "/var/lib/novacpx/panel.db")
+  [[ -z "$REPO_PATH" ]] && continue
-  CHANNEL=$(sqlite3 "$DB_PATH" "SELECT value FROM settings WHERE key='update_channel'" 2>/dev/null || echo "stable")
+
+  # Use branch recorded in queue entry (from webhook); fall back to DB channel
   TARGET_BRANCH="main"
-  [[ "$CHANNEL" == "beta" ]] && TARGET_BRANCH="beta"
+  if [[ "$QUEUED_BRANCH" == "beta" || ("$QUEUED_BRANCH" != "main" && "$CHANNEL" == "beta") ]]; then
+    TARGET_BRANCH="beta"
+  fi
+
+  log "--- Deploying commit $COMMIT (branch: $TARGET_BRANCH) ---"
 
   # Validate PHP syntax before applying
   cd "$REPO_PATH" || continue
@@ -47,9 +53,9 @@ while IFS='|' read -r REPO_PATH WEB_ROOT COMMIT; do
   fi
 
   # Pull from channel branch
-  BEFORE=$(git rev-parse HEAD)
+  BEFORE=$(git rev-parse HEAD | tr -cd 'a-f0-9A-F')
   git pull origin "${TARGET_BRANCH}" >> "$LOG" 2>&1
-  AFTER=$(git rev-parse HEAD)
+  AFTER=$(git rev-parse HEAD | tr -cd 'a-f0-9A-F')
 
   if [[ "$BEFORE" == "$AFTER" ]]; then
     log "Nothing new to deploy (already at $AFTER)"
@@ -88,8 +94,8 @@ while IFS='|' read -r REPO_PATH WEB_ROOT COMMIT; do
     done
   fi
 
-  # Record new version in DB
+  # Record new version in DB — sanitize values before interpolating into SQL
-  NEW_VERSION=$(cat "$REPO_PATH/VERSION" 2>/dev/null | tr -d '[:space:]' || true)
+  NEW_VERSION=$(cat "$REPO_PATH/VERSION" 2>/dev/null | tr -d '[:space:]' | tr -cd 'a-zA-Z0-9.-' || true)
   if [[ -n "$NEW_VERSION" && -f "$DB_PATH" ]]; then
     sqlite3 "$DB_PATH" "INSERT INTO novacpx_version (version, installed_at, notes, git_commit) VALUES ('$NEW_VERSION', datetime('now'), 'Auto-deployed via webhook ($CHANNEL channel)', '$AFTER')" 2>/dev/null || true
     sqlite3 "$DB_PATH" "INSERT INTO settings (key, value, updated_at) VALUES ('panel_version', '$NEW_VERSION', datetime('now')) ON CONFLICT(key) DO UPDATE SET value=excluded.value, updated_at=excluded.updated_at" 2>/dev/null || true

deploy/webhook.php

@@ -11,7 +11,6 @@ $cfg        = is_file($configFile) ? parse_ini_file($configFile, true) : [];
 $secret     = $cfg['deploy']['webhook_secret'] ?? '';
 $repoPath   = $cfg['deploy']['repo_path']      ?? '/opt/novacpx-src';
 $webRoot    = $cfg['deploy']['web_root']       ?? '/srv/novacpx/public';
-$branch     = $cfg['deploy']['branch']         ?? 'main';
 $logFile    = '/var/log/novacpx/deploy.log';
 
 header('Content-Type: application/json');
@@ -50,9 +49,9 @@ $message = $payload['head_commit']['message'] ?? '';
 
 log_deploy("Deploy triggered by $pusher | branch $pushedBranch | commit $commit | $message");
 
-// Queue the deploy — include branch so runner knows what to pull
+// Queue the deploy — include branch so runner uses the exact pushed branch
 $queueFile = '/tmp/novacpx-deploy-queue.txt';
-file_put_contents($queueFile, "$repoPath|$webRoot|$commit\n", FILE_APPEND | LOCK_EX);
+file_put_contents($queueFile, "$repoPath|$webRoot|$commit|$pushedBranch\n", FILE_APPEND | LOCK_EX);
 
 http_response_code(200);
 echo json_encode(['status' => 'queued', 'commit' => $commit]);

panel/api/endpoints/system.php

@@ -82,7 +82,7 @@ match ($action) {
                     $already = $db->fetchOne("SELECT 1 FROM settings WHERE `key` = 'migration_$migName'");
                     if (!$already) {
                         $db->pdo()->exec(file_get_contents($sql));
-                        $db->execute("INSERT INTO settings (`key`,`value`) VALUES (?,NOW()) ON DUPLICATE KEY UPDATE `value`=NOW()", ["migration_$migName"]);
+                        $db->execute("INSERT INTO settings (`key`,`value`,updated_at) VALUES (?,datetime('now'),datetime('now')) ON CONFLICT(`key`) DO UPDATE SET value=excluded.value,updated_at=excluded.updated_at", ["migration_$migName"]);
                         novacpx_log('info', "Migration applied: $migName");
                     }
                 }
@@ -221,14 +221,15 @@ BASH;
 
         $srcDir  = '/opt/novacpx-src';
         if (!is_dir($srcDir)) Response::error('Source repo not found at /opt/novacpx-src');
-        $channel = trim($db->fetchOne("SELECT value FROM settings WHERE `key`='update_channel'")['value'] ?? 'stable');
+        $channelRow   = $db->fetchOne("SELECT value FROM settings WHERE `key`='update_channel'");
+        $channel      = in_array($channelRow['value'] ?? '', ['stable', 'beta']) ? $channelRow['value'] : 'stable';
         $remoteBranch = $channel === 'beta' ? 'origin/beta' : 'origin/main';
         shell_exec("sudo git -C " . escapeshellarg($srcDir) . " fetch origin 2>/dev/null");
-        $logOut  = shell_exec("sudo git -C " . escapeshellarg($srcDir) . " log HEAD..{$remoteBranch} --oneline 2>/dev/null") ?: '';
+        $logOut  = shell_exec("sudo git -C " . escapeshellarg($srcDir) . " log HEAD.." . escapeshellarg($remoteBranch) . " --oneline 2>/dev/null") ?: '';
         $updates = array_values(array_filter(explode("\n", trim($logOut))));
         $branch  = trim(shell_exec("sudo git -C " . escapeshellarg($srcDir) . " branch --show-current 2>/dev/null") ?: 'main');
         $commit  = trim(shell_exec("sudo git -C " . escapeshellarg($srcDir) . " rev-parse --short HEAD 2>/dev/null") ?: '');
-        $remoteVer = trim(shell_exec("sudo git -C " . escapeshellarg($srcDir) . " show {$remoteBranch}:VERSION 2>/dev/null") ?: '');
+        $remoteVer = trim(shell_exec("sudo git -C " . escapeshellarg($srcDir) . " show " . escapeshellarg("{$remoteBranch}:VERSION") . " 2>/dev/null") ?: '');
         $result  = ['updates_available' => count($updates), 'current_commit' => $commit, 'branch' => $branch, 'channel' => $channel, 'remote_version' => $remoteVer, 'commits' => $updates];
         $db->execute("INSERT INTO settings(`key`,value,updated_at) VALUES('update_cache_novacpx',?,datetime('now')) ON CONFLICT(`key`) DO UPDATE SET value=excluded.value,updated_at=excluded.updated_at", [json_encode($result)]);
         Response::success($result);
@@ -244,15 +245,17 @@ BASH;
 
         if (!is_dir($srcDir)) Response::error('Source repo not found at /opt/novacpx-src');
 
-        $channel      = trim($db->fetchOne("SELECT value FROM settings WHERE `key`='update_channel'")['value'] ?? 'stable');
+        $channelRow   = $db->fetchOne("SELECT value FROM settings WHERE `key`='update_channel'");
+        $channel      = in_array($channelRow['value'] ?? '', ['stable', 'beta']) ? $channelRow['value'] : 'stable';
         $targetBranch = $channel === 'beta' ? 'beta' : 'main';
 
         $before = trim(shell_exec("sudo git -C " . escapeshellarg($srcDir) . " rev-parse HEAD 2>/dev/null") ?: '');
         $steps[] = "Before: $before (channel: $channel)";
 
         // Backup current web root to /tmp (writable, no sudo needed)
+        // rsync contents into $backupDir so restore can rsync $backupDir/ back symmetrically
         $backupDir = '/tmp/novacpx-backup-' . date('YmdHis');
-        shell_exec("cp -a " . escapeshellarg($webRoot) . " " . escapeshellarg($backupDir) . " 2>&1");
+        shell_exec("rsync -a " . escapeshellarg("$webRoot/") . " " . escapeshellarg("$backupDir/") . " 2>&1");
         $steps[] = "Backup: $backupDir";
 
         // Pull new code from the channel branch (sudo so www-data can write root-owned repo)
@@ -264,10 +267,16 @@ BASH;
         $steps[] = "After: $after" . ($changed ? " (changed)" : " (no change)");
 
         if ($changed) {
-            // Validate PHP syntax
+            // Validate PHP syntax — only check files changed in this update
+            $diffOut  = shell_exec("sudo git -C " . escapeshellarg($srcDir) . " diff " . escapeshellarg($before) . " " . escapeshellarg($after) . " --name-only 2>/dev/null") ?: '';
             $phpFiles = [];
-            $found = shell_exec("find " . escapeshellarg("$srcDir/panel") . " -name '*.php' 2>/dev/null") ?: '';
+            foreach (array_filter(explode("\n", trim($diffOut))) as $f) {
-            foreach (array_filter(explode("\n", trim($found))) as $f) { $phpFiles[] = trim($f); }
+                $f = trim($f);
+                if (str_ends_with($f, '.php')) {
+                    $full = "$srcDir/$f";
+                    if (file_exists($full)) $phpFiles[] = $full;
+                }
+            }
 
             $syntaxErr = [];
             foreach ($phpFiles as $f) {
@@ -276,7 +285,7 @@ BASH;
                     $syntaxErr[] = basename($f) . ': ' . trim($check);
                 }
             }
-            $steps[] = "Syntax check: " . count($phpFiles) . " files, " . count($syntaxErr) . " errors";
+            $steps[] = "Syntax check: " . count($phpFiles) . " changed files, " . count($syntaxErr) . " errors";
 
             if ($syntaxErr) {
                 shell_exec("sudo git -C " . escapeshellarg($srcDir) . " reset --hard " . escapeshellarg($before) . " 2>&1");
@@ -287,7 +296,7 @@ BASH;
             shell_exec("sudo rsync -a --delete " . escapeshellarg("$srcDir/panel/public/") . " " . escapeshellarg("$webRoot/") . " 2>&1");
             shell_exec("sudo rsync -a " . escapeshellarg("$srcDir/panel/lib/") . " " . escapeshellarg("$webRoot/lib/") . " 2>&1");
             shell_exec("sudo rsync -a " . escapeshellarg("$srcDir/panel/api/") . " " . escapeshellarg("$webRoot/api/") . " 2>&1");
-            shell_exec("sudo cp " . escapeshellarg("$srcDir/VERSION") . " " . escapeshellarg("$webRoot/../VERSION") . " 2>/dev/null");
+            shell_exec("sudo cp " . escapeshellarg("$srcDir/VERSION") . " " . escapeshellarg("$webRoot/VERSION") . " 2>/dev/null");
             shell_exec("sudo chown -R www-data:www-data " . escapeshellarg($webRoot));
             $steps[] = "Deploy: rsync complete";
 
@@ -584,7 +593,7 @@ BASH;
         };
 
         // Persist selection
-        $db->execute("INSERT INTO settings (`key`,`value`) VALUES (?,?) ON DUPLICATE KEY UPDATE `value`=VALUES(`value`)", [$key, $value]);
+        $db->execute("INSERT INTO settings (`key`,`value`) VALUES (?,?) ON CONFLICT(`key`) DO UPDATE SET value=excluded.value", [$key, $value]);
 
         // Sync config.ini
         $configFile = '/etc/novacpx/config.ini';
@@ -700,7 +709,7 @@ BASH;
             $value = trim($body[$key]);
             if ($key === 'cybermail_api_key' && str_contains($value, '***')) continue; // skip masked placeholder
             $db->execute(
-                "INSERT INTO settings (`key`,`value`) VALUES (?,?) ON DUPLICATE KEY UPDATE `value`=VALUES(`value`)",
+                "INSERT INTO settings (`key`,`value`) VALUES (?,?) ON CONFLICT(`key`) DO UPDATE SET value=excluded.value",
                 [$key, $value]
             );
             $saved[] = $key;
@@ -897,7 +906,7 @@ BASH;
             shell_exec("sudo systemctl stop $engine 2>/dev/null || true");
             $out = shell_exec("sudo env DEBIAN_FRONTEND=noninteractive apt-get remove -y $pkg 2>&1");
         } elseif ($action === 'set-active') {
-            $db->execute("INSERT INTO settings (`key`,`value`) VALUES ('active_db_engine',?) ON DUPLICATE KEY UPDATE `value`=VALUES(`value`)", [$engine]);
+            $db->execute("INSERT INTO settings (`key`,`value`) VALUES ('active_db_engine',?) ON CONFLICT(`key`) DO UPDATE SET value=excluded.value", [$engine]);
             audit('settings.active_db_engine', $engine);
             Response::success(null, "Active database engine set to $engine");
         } else {