diff --git a/.gitea/workflows/security.yml b/.gitea/workflows/security.yml index c8b7c11..71b8542 100644 --- a/.gitea/workflows/security.yml +++ b/.gitea/workflows/security.yml @@ -19,7 +19,9 @@ jobs: run: | apt-get update -qq apt-get install -y -qq python3 python3-pip - pip3 install semgrep + # Debian's Python is externally managed (PEP 668); the runner is + # ephemeral so installing system-wide is fine here. + pip3 install --break-system-packages semgrep - name: Run semgrep run: | diff --git a/create_ticket_api.php b/create_ticket_api.php index 8ffa930..24dd618 100644 --- a/create_ticket_api.php +++ b/create_ticket_api.php @@ -45,9 +45,11 @@ $conn = new mysqli( ); if ($conn->connect_error) { + error_log('create_ticket_api: DB connection failed: ' . $conn->connect_error); + http_response_code(500); echo json_encode([ 'success' => false, - 'error' => 'Database connection failed: ' . $conn->connect_error + 'error' => 'Internal server error' ]); exit; } @@ -199,6 +201,11 @@ function generateTicketHash($data) )), ]; + // Manual tickets should be unique by title (so different software installs don't collide) + if ($sourceType === 'manual') { + $stableComponents['title'] = $title; + } + // Include hostname for node-specific issues if (!$isClusterWide) { $stableComponents['hostname'] = $hostname; @@ -397,7 +404,9 @@ try { // Race condition: another node inserted the same hash between our SELECT and INSERT echo json_encode(['success' => false, 'error' => 'Duplicate ticket']); } else { - echo json_encode(['success' => false, 'error' => $e->getMessage()]); + error_log('create_ticket_api: insert failed: ' . $e->getMessage()); + http_response_code(500); + echo json_encode(['success' => false, 'error' => 'Internal server error']); } exit; } diff --git a/cron/create_recurring_tickets.php b/cron/create_recurring_tickets.php index f569570..b5109e7 100644 --- a/cron/create_recurring_tickets.php +++ b/cron/create_recurring_tickets.php @@ -16,6 +16,7 @@ chdir(dirname(__DIR__)); // Include required files require_once 'config/config.php'; +require_once 'helpers/Database.php'; require_once 'models/RecurringTicketModel.php'; require_once 'models/TicketModel.php'; require_once 'models/AuditLogModel.php'; @@ -29,17 +30,9 @@ function logMessage($message) logMessage("Starting recurring tickets cron job"); try { - // Create database connection - $conn = new mysqli( - $GLOBALS['config']['DB_HOST'], - $GLOBALS['config']['DB_USER'], - $GLOBALS['config']['DB_PASS'], - $GLOBALS['config']['DB_NAME'] - ); - - if ($conn->connect_error) { - throw new Exception("Database connection failed: " . $conn->connect_error); - } + // Create database connection (Database::getConnection sets utf8mb4 so + // non-ASCII titles/descriptions aren't corrupted on insert). + $conn = Database::getConnection(); // Initialize models $recurringModel = new RecurringTicketModel($conn); @@ -57,6 +50,14 @@ try { logMessage("Processing recurring ticket ID: " . $recurring['recurring_id']); try { + // Claim the schedule FIRST (atomic advance of next_run_at). If another + // cron run already claimed it, or it's no longer due, skip it — this + // prevents duplicate-ticket floods if a later step throws. + if (!$recurringModel->claimForRun($recurring['recurring_id'])) { + logMessage("Skipped (already claimed or not due): " . $recurring['recurring_id']); + continue; + } + // Prepare ticket data $ticketData = [ 'title' => processTemplate($recurring['title_template']), @@ -74,9 +75,12 @@ try { $ticketId = $result['ticket_id']; logMessage("Created ticket: " . $ticketId); - // Assign to user if specified - if ($recurring['assigned_to']) { - $ticketModel->assignTicket($ticketId, $recurring['assigned_to'], $recurring['created_by']); + // Assign to user if specified. assignTicket() requires a non-null + // "assigned_by"; fall back to the assignee when created_by is null + // (recurring schedules may have no creator). + if (!empty($recurring['assigned_to'])) { + $assignedBy = (int)($recurring['created_by'] ?? $recurring['assigned_to']); + $ticketModel->assignTicket($ticketId, (int)$recurring['assigned_to'], $assignedBy); } // Log to audit @@ -88,9 +92,6 @@ try { ['source' => 'recurring', 'recurring_id' => $recurring['recurring_id']] ); - // Update the recurring ticket's next run time - $recurringModel->updateAfterRun($recurring['recurring_id']); - $created++; } else { logMessage("ERROR: Failed to create ticket - " . ($result['error'] ?? 'Unknown error')); @@ -104,7 +105,7 @@ try { logMessage("Completed: Created $created tickets, $errors errors"); - $conn->close(); + Database::close(); } catch (Exception $e) { logMessage("FATAL ERROR: " . $e->getMessage()); exit(1); diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 7e6d283..ec1a383 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -94,6 +94,10 @@ class BulkOperationsModel // Start transaction for data consistency $this->conn->begin_transaction(); + // Attachment files for deleted tickets are removed only AFTER a successful + // commit, so a rollback can't leave tickets with their files already gone. + $filesToDelete = []; + try { foreach ($ticketIds as $ticketId) { $ticketId = trim($ticketId); @@ -200,7 +204,7 @@ class BulkOperationsModel break; case 'bulk_delete': - $success = $ticketModel->deleteTicket($ticketId); + $success = $ticketModel->deleteTicket($ticketId, $filesToDelete); if ($success) { $auditLogModel->log( $operation['performed_by'], @@ -249,6 +253,16 @@ class BulkOperationsModel // Commit the transaction $this->conn->commit(); + + // Now that the DB delete is durable, remove the physical files. Files + // are deleted first; directory entries (no trailing filename) last. + foreach ($filesToDelete as $path) { + if (is_dir($path)) { + @rmdir($path); // only succeeds if empty + } elseif (file_exists($path)) { + @unlink($path); + } + } } catch (Exception $e) { // Rollback on any unexpected error $this->conn->rollback(); diff --git a/models/RecurringTicketModel.php b/models/RecurringTicketModel.php index 6eeecd0..2e54b38 100644 --- a/models/RecurringTicketModel.php +++ b/models/RecurringTicketModel.php @@ -151,6 +151,44 @@ class RecurringTicketModel return $items; } + /** + * Atomically claim a due schedule for processing. + * + * Advances next_run_at (and stamps last_run_at) in a single conditional + * UPDATE gated on the row still being active and due. Returns true only if + * THIS call won the claim. This must be done BEFORE creating the ticket so + * that: + * - two overlapping cron runs can't both process the same schedule, and + * - a failure in a later step (ticket create, assignment, audit) can't + * leave next_run_at in the past, which would re-fire — and re-create a + * duplicate ticket — on every subsequent cron run. + * + * @return bool true if the schedule was claimed by this call + */ + public function claimForRun($recurringId) + { + $recurring = $this->getById($recurringId); + if (!$recurring) { + return false; + } + + $nextRun = $this->calculateNextRunTime( + $recurring['schedule_type'], + $recurring['schedule_day'], + $recurring['schedule_time'] + ); + + $sql = "UPDATE recurring_tickets + SET last_run_at = NOW(), next_run_at = ? + WHERE recurring_id = ? AND is_active = 1 AND next_run_at <= NOW()"; + $stmt = $this->conn->prepare($sql); + $stmt->bind_param('si', $nextRun, $recurringId); + $stmt->execute(); + $claimed = $stmt->affected_rows > 0; + $stmt->close(); + return $claimed; + } + /** * Update last run and calculate next run time */ diff --git a/models/TicketModel.php b/models/TicketModel.php index c56f976..23506c3 100644 --- a/models/TicketModel.php +++ b/models/TicketModel.php @@ -740,9 +740,13 @@ class TicketModel * Admin-only operation. Removes comments, attachments, watchers, dependencies. * * @param string $ticketId Ticket ID + * @param array|null &$deferredFiles When provided, attachment file paths to + * remove are appended here instead of being unlinked immediately, so a + * caller running inside a DB transaction can delete them only AFTER a + * successful commit (avoids destroying files for a rolled-back delete). * @return bool Success status */ - public function deleteTicket(string $ticketId): bool + public function deleteTicket(string $ticketId, ?array &$deferredFiles = null): bool { // Collect attachment filenames before deleting DB rows $attachmentFiles = []; @@ -804,13 +808,21 @@ class TicketModel : (isset($GLOBALS['config']['UPLOAD_DIR']) ? $GLOBALS['config']['UPLOAD_DIR'] : dirname(__DIR__) . '/uploads'); $ticketDir = rtrim($uploadDir, '/') . '/' . $ticketId; if (is_dir($ticketDir)) { - foreach ($attachmentFiles as $filename) { - $file = $ticketDir . '/' . basename($filename); - if (file_exists($file)) { - @unlink($file); + if ($deferredFiles !== null) { + // Defer physical deletion to the caller (post-commit). + foreach ($attachmentFiles as $filename) { + $deferredFiles[] = $ticketDir . '/' . basename($filename); } + $deferredFiles[] = $ticketDir; // dir removed last, only if empty + } else { + foreach ($attachmentFiles as $filename) { + $file = $ticketDir . '/' . basename($filename); + if (file_exists($file)) { + @unlink($file); + } + } + @rmdir($ticketDir); // Remove dir only if empty } - @rmdir($ticketDir); // Remove dir only if empty } return true; } diff --git a/models/UserModel.php b/models/UserModel.php index 219af60..0664d94 100644 --- a/models/UserModel.php +++ b/models/UserModel.php @@ -86,7 +86,7 @@ class UserModel $user = $result->fetch_assoc(); $updateStmt = $this->conn->prepare( - "UPDATE users SET display_name = ?, email = ?, groups = ?, is_admin = ?, last_login = NOW() WHERE username = ?" + "UPDATE users SET display_name = ?, email = ?, `groups` = ?, is_admin = ?, last_login = NOW() WHERE username = ?" ); $updateStmt->bind_param("sssis", $displayName, $email, $groups, $isAdmin, $username); $updateStmt->execute(); @@ -100,7 +100,7 @@ class UserModel } else { // Create new user $insertStmt = $this->conn->prepare( - "INSERT INTO users (username, display_name, email, groups, is_admin, last_login) VALUES (?, ?, ?, ?, ?, NOW())" + "INSERT INTO users (username, display_name, email, `groups`, is_admin, last_login) VALUES (?, ?, ?, ?, ?, NOW())" ); $insertStmt->bind_param("ssssi", $username, $displayName, $email, $groups, $isAdmin); $insertStmt->execute(); @@ -300,7 +300,7 @@ class UserModel return $cached; } - $stmt = $this->conn->prepare("SELECT DISTINCT groups FROM users WHERE groups IS NOT NULL AND groups != ''"); + $stmt = $this->conn->prepare("SELECT DISTINCT `groups` FROM users WHERE `groups` IS NOT NULL AND `groups` != ''"); $stmt->execute(); $result = $stmt->get_result();