Merge development into main: high-priority security/reliability batch (#27, #28, #30, #32)

- Add missing rate limiting to create_ticket_api.php (#27)
- Fix visibility-group matching disagreement between filter and access check (#28)
- Replace illusory transaction wrapping in migrate.php with statement-level resume (#30)
- Fix ticket_watchers.ticket_id type mismatch and missing FK (#32)

api/notifications.php

@@ -138,7 +138,7 @@ $statusSql = "SELECT DISTINCT
     COALESCE(u.display_name, u.username, 'System') AS actor_name
   FROM audit_log al
   LEFT JOIN users u ON al.user_id = u.user_id
-  INNER JOIN ticket_watchers tw ON tw.ticket_id = CAST(al.entity_id AS UNSIGNED) AND tw.user_id = ?
+  INNER JOIN ticket_watchers tw ON tw.ticket_id = al.entity_id AND tw.user_id = ?
   WHERE al.action_type = 'update'
     AND al.entity_type = 'ticket'
     AND al.user_id != ?

api/watch_ticket.php

@@ -12,40 +12,43 @@ require_once dirname(__DIR__) . '/models/TicketModel.php';
 
 $data = json_decode(file_get_contents('php://input'), true) ?? [];
 
-$ticketId = isset($_GET['ticket_id'])
+$ticketIdRaw = isset($_GET['ticket_id']) ? $_GET['ticket_id'] : ($data['ticket_id'] ?? '');
-    ? (int)$_GET['ticket_id']
-    : (int)($data['ticket_id'] ?? 0);
 
 if ($_SERVER['REQUEST_METHOD'] === 'POST') {
-    $ticketId = (int)($data['ticket_id'] ?? 0);
+    $ticketIdRaw = $data['ticket_id'] ?? '';
-    $action   = $data['action'] ?? '';
+    $action      = $data['action'] ?? '';
 
-    if ($ticketId <= 0 || !in_array($action, ['watch', 'unwatch'], true)) {
+    if ($ticketIdRaw === '' || !in_array($action, ['watch', 'unwatch'], true)) {
         http_response_code(400);
         echo json_encode(['success' => false, 'error' => 'Invalid parameters']);
         exit;
     }
 
     $ticketModel = new TicketModel($conn);
-    $ticket = $ticketModel->getTicketById($ticketId);
+    $ticket = $ticketModel->getTicketById((string)$ticketIdRaw);
     if (!$ticket || !$ticketModel->canUserAccessTicket($ticket, $currentUser)) {
         http_response_code(404);
         echo json_encode(['success' => false, 'error' => 'Ticket not found']);
         exit;
     }
 
+    // Use the canonical ticket_id string from the fetched ticket row, not the
+    // raw request value, so ticket_watchers always stores exactly what's in
+    // tickets.ticket_id.
+    $ticketId = $ticket['ticket_id'];
+
     if ($action === 'watch') {
         $stmt = $conn->prepare(
             "INSERT IGNORE INTO ticket_watchers (ticket_id, user_id) VALUES (?, ?)"
         );
-        $stmt->bind_param("ii", $ticketId, $userId);
+        $stmt->bind_param("si", $ticketId, $userId);
         $stmt->execute();
         $stmt->close();
     } else {
         $stmt = $conn->prepare(
             "DELETE FROM ticket_watchers WHERE ticket_id = ? AND user_id = ?"
         );
-        $stmt->bind_param("ii", $ticketId, $userId);
+        $stmt->bind_param("si", $ticketId, $userId);
         $stmt->execute();
         $stmt->close();
     }
@@ -54,7 +57,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') {
     $countStmt = $conn->prepare(
         "SELECT COUNT(*) as cnt FROM ticket_watchers WHERE ticket_id = ?"
     );
-    $countStmt->bind_param("i", $ticketId);
+    $countStmt->bind_param("s", $ticketId);
     $countStmt->execute();
     $count = (int)$countStmt->get_result()->fetch_assoc()['cnt'];
     $countStmt->close();
@@ -73,7 +76,7 @@ if ($_SERVER['REQUEST_METHOD'] !== 'GET') {
     exit;
 }
 
-if ($ticketId <= 0) {
+if ($ticketIdRaw === '') {
     http_response_code(400);
     echo json_encode(['success' => false, 'error' => 'ticket_id required']);
     exit;
@@ -83,17 +86,19 @@ if ($ticketId <= 0) {
 // restricted ticket's watcher list and count aren't disclosed (the POST path
 // already checks this).
 $ticketModel = new TicketModel($conn);
-$ticket = $ticketModel->getTicketById($ticketId);
+$ticket = $ticketModel->getTicketById((string)$ticketIdRaw);
 if (!$ticket || !$ticketModel->canUserAccessTicket($ticket, $currentUser)) {
     http_response_code(404);
     echo json_encode(['success' => false, 'error' => 'Ticket not found']);
     exit;
 }
 
+$ticketId = $ticket['ticket_id'];
+
 $watchingStmt = $conn->prepare(
     "SELECT COUNT(*) as cnt FROM ticket_watchers WHERE ticket_id = ? AND user_id = ?"
 );
-$watchingStmt->bind_param("ii", $ticketId, $userId);
+$watchingStmt->bind_param("si", $ticketId, $userId);
 $watchingStmt->execute();
 $watching = (bool)$watchingStmt->get_result()->fetch_assoc()['cnt'];
 $watchingStmt->close();
@@ -107,7 +112,7 @@ $watchersStmt = $conn->prepare(
       ORDER BY tw.created_at ASC
       LIMIT 6"
 );
-$watchersStmt->bind_param("i", $ticketId);
+$watchersStmt->bind_param("s", $ticketId);
 $watchersStmt->execute();
 $watchersResult = $watchersStmt->get_result();
 $watchers = [];
@@ -118,7 +123,7 @@ $watchersStmt->close();
 
 // True watcher count (the list above is capped at 6 for the avatar group)
 $countStmt = $conn->prepare("SELECT COUNT(*) AS cnt FROM ticket_watchers WHERE ticket_id = ?");
-$countStmt->bind_param("i", $ticketId);
+$countStmt->bind_param("s", $ticketId);
 $countStmt->execute();
 $count = (int)$countStmt->get_result()->fetch_assoc()['cnt'];
 $countStmt->close();

create_ticket_api.php

@@ -5,6 +5,9 @@ header('Content-Type: application/json');
 error_reporting(E_ALL);
 ini_set('display_errors', 0);
 
+require_once __DIR__ . '/middleware/RateLimitMiddleware.php';
+RateLimitMiddleware::apply('api');
+
 // Load environment variables with error check
 $envFile = __DIR__ . '/.env';
 if (!file_exists($envFile)) {

helpers/NotificationHelper.php

@@ -204,9 +204,9 @@ class NotificationHelper
                 return;
             }
             if ($excludeUserId !== null) {
-                $stmt->bind_param("ii", $ticketId, $excludeUserId);
+                $stmt->bind_param("si", $ticketId, $excludeUserId);
             } else {
-                $stmt->bind_param("i", $ticketId);
+                $stmt->bind_param("s", $ticketId);
             }
             $stmt->execute();
             $result = $stmt->get_result();

migrations/000_baseline.sql

@@ -243,11 +243,12 @@ CREATE TABLE IF NOT EXISTS `ticket_templates` (
 
 -- ============ ticket_watchers ============
 CREATE TABLE IF NOT EXISTS `ticket_watchers` (
-  `ticket_id` int(11) NOT NULL,
+  `ticket_id` varchar(9) NOT NULL,
   `user_id` int(11) NOT NULL,
   `created_at` timestamp NOT NULL DEFAULT current_timestamp(),
   PRIMARY KEY (`ticket_id`,`user_id`),
-  KEY `idx_watcher_user` (`user_id`)
+  KEY `idx_watcher_user` (`user_id`),
+  CONSTRAINT `fk_watchers_ticket_id` FOREIGN KEY (`ticket_id`) REFERENCES `tickets` (`ticket_id`) ON DELETE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci;
 
 -- ============ tickets ============

migrations/004_fix_ticket_watchers_type.sql

@@ -0,0 +1,28 @@
+-- Fix ticket_watchers.ticket_id type mismatch and missing FK to tickets
+--
+-- ticket_watchers.ticket_id was int(11), while every other satellite table
+-- (ticket_comments, ticket_attachments, ticket_dependencies,
+-- custom_field_values) stores it as varchar(9)/varchar(10) matching
+-- tickets.ticket_id. There was also no FK constraint at all, unlike every
+-- other satellite table, so orphaned watcher rows could never be caught by
+-- referential integrity. Ticket IDs are always 9-digit numeric strings
+-- (see TicketModel::create's sprintf('%09d', ...)), so the int -> varchar(9)
+-- conversion below is lossless for real data.
+--
+-- Safe to re-run.
+
+-- Remove any watcher rows that no longer point at a real ticket (possible
+-- today precisely because there was no FK to prevent it) before adding the
+-- constraint, since orphans would make the ADD CONSTRAINT below fail.
+DELETE tw FROM `ticket_watchers` tw
+    LEFT JOIN `tickets` t ON tw.`ticket_id` = t.`ticket_id`
+    WHERE t.`ticket_id` IS NULL;
+
+ALTER TABLE `ticket_watchers`
+    MODIFY COLUMN `ticket_id` varchar(9) NOT NULL;
+
+ALTER TABLE `ticket_watchers`
+    DROP FOREIGN KEY IF EXISTS `fk_watchers_ticket_id`;
+
+ALTER TABLE `ticket_watchers`
+    ADD CONSTRAINT `fk_watchers_ticket_id` FOREIGN KEY (`ticket_id`) REFERENCES `tickets` (`ticket_id`) ON DELETE CASCADE;

migrations/migrate.php

@@ -46,6 +46,23 @@ if (!$conn->query($createTable)) {
     exit(1);
 }
 
+// Tracks per-statement progress within a migration file. MySQL DDL statements
+// (ALTER/CREATE TABLE, etc.) cause an implicit commit, so begin_transaction()/
+// rollback() around a whole file can't actually undo DDL already executed
+// earlier in that same file. This table lets a re-run after a partial failure
+// resume from the statement after the last one that succeeded, instead of
+// re-executing already-applied DDL and wedging on "already exists" errors.
+$createProgressTable = "CREATE TABLE IF NOT EXISTS migration_progress (
+    filename VARCHAR(255) NOT NULL PRIMARY KEY,
+    last_statement_index INT NOT NULL,
+    updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
+)";
+
+if (!$conn->query($createProgressTable)) {
+    echo "Error: Could not create migration_progress table: " . $conn->error . "\n";
+    exit(1);
+}
+
 // Get list of completed migrations
 $completed = [];
 $result = $conn->query("SELECT filename FROM migrations ORDER BY id");
@@ -114,47 +131,87 @@ foreach ($pending as $file) {
         continue;
     }
 
-    // Execute migration - handle multiple statements
+    // Execute migration statement-by-statement, tracking progress as we go.
-    $conn->begin_transaction();
+    // No begin_transaction()/rollback() here: DDL statements auto-commit in
-
+    // MySQL/MariaDB regardless, so a transaction wrapper around the whole
+    // file would only create the illusion of atomicity while giving no real
+    // protection. Instead, each statement commits immediately (autocommit),
+    // and its index is durably recorded so a later re-run can resume exactly
+    // where a previous run left off rather than re-executing already-applied
+    // DDL.
     try {
         // Split by semicolon but respect statements properly
         // Note: This doesn't handle semicolons in strings, but our migrations are simple
-        $statements = array_filter(
+        $statements = array_values(array_filter(
             array_map('trim', explode(';', $sql)),
             function($stmt) {
                 // Remove comments and check if there's actual SQL
                 $cleaned = preg_replace('/--.*$/m', '', $stmt);
                 return !empty(trim($cleaned));
             }
-        );
+        ));
+
+        $resumeFrom = 0;
+        $progressStmt = $conn->prepare(
+            "SELECT last_statement_index FROM migration_progress WHERE filename = ?"
+        );
+        $progressStmt->bind_param('s', $filename);
+        $progressStmt->execute();
+        $progressRow = $progressStmt->get_result()->fetch_assoc();
+        $progressStmt->close();
+        if ($progressRow) {
+            $resumeFrom = (int)$progressRow['last_statement_index'] + 1;
+            echo "\n  Resuming from statement " . ($resumeFrom + 1) . " of " . count($statements)
+                . " after a previous partial failure... ";
+        }
+
+        foreach ($statements as $index => $statement) {
+            if ($index < $resumeFrom) {
+                continue;
+            }
 
-        foreach ($statements as $statement) {
             if (!$conn->query($statement)) {
                 // Some "errors" are acceptable (like "index already exists")
                 $error = $conn->error;
                 if (strpos($error, 'Duplicate key name') !== false ||
                     strpos($error, 'already exists') !== false) {
                     // Index already exists, that's fine
-                    continue;
+                } else {
+                    throw new Exception($error);
                 }
-                throw new Exception($error);
             }
+
+            // Record progress after every statement so a later run can
+            // resume from here even if a subsequent statement fails.
+            $upsert = $conn->prepare(
+                "INSERT INTO migration_progress (filename, last_statement_index) VALUES (?, ?)
+                 ON DUPLICATE KEY UPDATE last_statement_index = VALUES(last_statement_index)"
+            );
+            $upsert->bind_param('si', $filename, $index);
+            $upsert->execute();
+            $upsert->close();
         }
 
-        // Record the migration
+        // Record the migration as fully complete and clear its progress marker
         $stmt = $conn->prepare("INSERT INTO migrations (filename) VALUES (?)");
         $stmt->bind_param('s', $filename);
         if (!$stmt->execute()) {
             throw new Exception("Could not record migration: " . $conn->error);
         }
 
-        $conn->commit();
+        $clearProgress = $conn->prepare("DELETE FROM migration_progress WHERE filename = ?");
+        $clearProgress->bind_param('s', $filename);
+        $clearProgress->execute();
+        $clearProgress->close();
+
         echo "OK\n";
         $success++;
 
     } catch (Exception $e) {
-        $conn->rollback();
+        // Nothing to roll back: every statement up to the failure already
+        // committed (DDL implicitly, everything else via autocommit). The
+        // progress marker recorded above reflects exactly how far this file
+        // got, so the next run will resume right after the last success.
         echo "FAILED (" . $e->getMessage() . ")\n";
         $failed++;
     }

models/TicketModel.php

@@ -726,7 +726,10 @@ class TicketModel
             $groupConditions = [];
             foreach ($userGroups as $group) {
                 $groupConditions[] = "FIND_IN_SET(?, REPLACE(t.visibility_groups, ' ', ''))";
-                $params[] = $group;
+                // Strip spaces from the bound value too, matching the REPLACE()
+                // applied to the column, so a group name like "IT Support" is
+                // normalized the same way on both sides of the comparison.
+                $params[] = str_replace(' ', '', $group);
                 $types .= 's';
             }
             $conditions[] = "(t.visibility = 'internal' AND (" . implode(' OR ', $groupConditions) . "))";