From a9a39adcf8b15263530b2b44b9a6b26c5ccab8bd Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 21:28:18 -0400 Subject: [PATCH 1/4] Add missing rate limiting to create_ticket_api.php (#27) Every other Bearer-key endpoint (ticket_status_api.php, ticket_comment_api.php) calls RateLimitMiddleware::apply('api') before opening a DB connection; create_ticket_api.php didn't, contradicting README.md's claim that the whole Bearer API is rate-limited. A leaked or guessed API key could hammer ticket creation unthrottled, each insert also firing a Matrix webhook. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP --- create_ticket_api.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/create_ticket_api.php b/create_ticket_api.php index 62b6fcc..e4a85a9 100644 --- a/create_ticket_api.php +++ b/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)) { From fd777aa69096633c77524df5d1cc55286b28318c Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 21:28:25 -0400 Subject: [PATCH 2/4] Fix visibility-group matching disagreement between filter and access check (#28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getVisibilityFilter() (dashboard list/stats) matched via FIND_IN_SET(?, REPLACE(t.visibility_groups, ' ', '')) — stripping spaces from the column but not from the bound group name — while canUserAccessTicket() (single-ticket access) did a plain trim with no space-stripping at all. For a group name containing a space (e.g. "IT Support"), a member could open an internal ticket directly by URL but never see it in their dashboard list or stats counts. Now strips spaces from the bound parameter too, matching the column- side normalization, so both paths agree. Verified against real MariaDB: a ticket visible via canUserAccessTicket() for a space-containing group is now also matched by getVisibilityFilter()'s SQL, a wrong-group user is denied by both, and the plain no-space case is unaffected. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP --- models/TicketModel.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/models/TicketModel.php b/models/TicketModel.php index b64c837..7e05e42 100644 --- a/models/TicketModel.php +++ b/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) . "))"; From fba251b85dc5764ac1516b0c9dbcf63597f86e01 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 21:28:32 -0400 Subject: [PATCH 3/4] Replace illusory transaction wrapping in migrate.php with statement-level resume (#30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit migrate.php wrapped each migration file's statements in begin_transaction()/rollback(), but MySQL DDL statements cause an implicit commit — so a rollback couldn't actually undo earlier DDL already executed within the same file. A migration failing partway left the DB altered but unrecorded, and the next run retried the whole file from statement 1, hitting "already exists" errors not on the safe-to-ignore allowlist and permanently wedging the runner. Removed the transaction wrapper (it only gave false confidence) and added a migration_progress table that records the index of the last successfully-executed statement in each file. A re-run after a partial failure now resumes right after the last success instead of re-executing already-applied DDL. Verified against real MariaDB with a 4-statement migration where statement 3 fails: run 1 correctly applies statements 1-2 and records progress at index 1; after fixing the bad statement, run 2 resumes at statement 3, completes, and clears the progress marker. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP --- migrations/migrate.php | 79 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 11 deletions(-) diff --git a/migrations/migrate.php b/migrations/migrate.php index a725242..b34a8b4 100644 --- a/migrations/migrate.php +++ b/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 - $conn->begin_transaction(); - + // Execute migration statement-by-statement, tracking progress as we go. + // 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++; } From d7940b1e312093a8c1237b509a888be5adfdbe92 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 21:28:42 -0400 Subject: [PATCH 4/4] Fix ticket_watchers.ticket_id type mismatch and missing FK (#32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ticket_watchers.ticket_id was int(11) while every other satellite table (ticket_comments, ticket_attachments, ticket_dependencies, custom_field_values) uses varchar(9)/varchar(10) matching tickets.ticket_id, and it had no FK constraint at all — unlike every other satellite table — so orphaned watcher rows could never be caught by referential integrity. Changed the column to varchar(9) with an ON DELETE CASCADE FK to tickets, in both 000_baseline.sql and a new idempotent 004_fix_ticket_watchers_type.sql (which also deletes any pre-existing orphaned watcher rows before adding the constraint, since orphans would otherwise make the ADD CONSTRAINT fail). Updated watch_ticket.php, NotificationHelper::notifyWatchers(), and notifications.php's audit-log JOIN to bind/compare ticket_id as a string instead of casting to int, including replacing a fragile CAST(entity_id AS UNSIGNED) with a direct string comparison. Verified against real MariaDB: applied 004 against a simulated pre-fix deployment with one valid and one orphaned watcher row — the orphan is removed, the column converts losslessly, the FK is added, and the migration is idempotent on re-run. Confirmed ON DELETE CASCADE actually removes watchers when their ticket is deleted, that inserting a watcher for a nonexistent ticket now fails with a real FK violation, and exercised the updated watch/unwatch and status-change-notification query paths end-to-end against the fixed schema. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015nCxwFFsy8ouMWzn56rPVP --- api/notifications.php | 2 +- api/watch_ticket.php | 35 ++++++++++++--------- helpers/NotificationHelper.php | 4 +-- migrations/000_baseline.sql | 5 +-- migrations/004_fix_ticket_watchers_type.sql | 28 +++++++++++++++++ 5 files changed, 54 insertions(+), 20 deletions(-) create mode 100644 migrations/004_fix_ticket_watchers_type.sql diff --git a/api/notifications.php b/api/notifications.php index e5c0586..ff22d70 100644 --- a/api/notifications.php +++ b/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 != ? diff --git a/api/watch_ticket.php b/api/watch_ticket.php index 10c7f3e..40e9867 100644 --- a/api/watch_ticket.php +++ b/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']) - ? (int)$_GET['ticket_id'] - : (int)($data['ticket_id'] ?? 0); +$ticketIdRaw = isset($_GET['ticket_id']) ? $_GET['ticket_id'] : ($data['ticket_id'] ?? ''); if ($_SERVER['REQUEST_METHOD'] === 'POST') { - $ticketId = (int)($data['ticket_id'] ?? 0); - $action = $data['action'] ?? ''; + $ticketIdRaw = $data['ticket_id'] ?? ''; + $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(); diff --git a/helpers/NotificationHelper.php b/helpers/NotificationHelper.php index 1afceac..af4cf91 100644 --- a/helpers/NotificationHelper.php +++ b/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(); diff --git a/migrations/000_baseline.sql b/migrations/000_baseline.sql index f5ffec9..02781a9 100644 --- a/migrations/000_baseline.sql +++ b/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 ============ diff --git a/migrations/004_fix_ticket_watchers_type.sql b/migrations/004_fix_ticket_watchers_type.sql new file mode 100644 index 0000000..c3d4d7e --- /dev/null +++ b/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;