From e0e92e326a8b6f87910d9c1954bfbc8b5748b96d Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 30 Jun 2026 12:26:28 -0400 Subject: [PATCH] Quick-win fixes from second review - create_ticket_api.php: validate status (against TICKET_STATUSES) and priority (numeric 1-5). A non-numeric priority previously cast to 0 and escalated the ticket below P1 on the dedup/update path. - manage_workflows.php: reject empty/invalid from_status/to_status on POST and PUT (must be valid ticket statuses) so the workflow table can't be populated with bogus transitions. - TicketModel::getAllTickets: COUNT(*) OVER() rides on returned rows, so a page past the last row returned total/pages = 0. Fall back to a direct COUNT when an over-range page yields no rows, keeping pager math correct. - DashboardView: stop double-escaping category/type/assigned active-filter labels (they were htmlspecialchars'd into the label and again at output, rendering R&D as R&D); output escaping is retained. - check_duplicates.php / NotificationHelper::notifyWatchers: wrap the DB lookups in try/catch so a failed prepare/query degrades gracefully (advisory dup-check returns none; best-effort watcher notify is skipped) instead of fataling the request. Works whether mysqli throws or returns false. (manage_* endpoints already have a top-level try/catch.) Co-Authored-By: Claude Opus 4.8 --- api/check_duplicates.php | 22 ++++++++++--- api/manage_workflows.php | 18 +++++++++++ create_ticket_api.php | 16 ++++++++++ helpers/NotificationHelper.php | 56 +++++++++++++++++++--------------- models/TicketModel.php | 23 ++++++++++++++ views/DashboardView.php | 6 ++-- 6 files changed, 108 insertions(+), 33 deletions(-) diff --git a/api/check_duplicates.php b/api/check_duplicates.php index 01c3728..1fb36d2 100644 --- a/api/check_duplicates.php +++ b/api/check_duplicates.php @@ -50,12 +50,24 @@ $sql = "SELECT ticket_id, title, status, priority, created_at $types = "ss" . $visFilter['types']; $params = array_merge([$searchTerm, $soundexTitle], $visFilter['params']); -$stmt = $conn->prepare($sql); -if (!empty($params)) { - $stmt->bind_param($types, ...$params); + +// Duplicate detection is advisory (it must not block ticket creation), so on any +// DB error degrade gracefully to "no duplicates" rather than fataling the request. +// mysqli may throw (default exception mode) or return false depending on config. +try { + $stmt = $conn->prepare($sql); + if (!$stmt) { + throw new RuntimeException('prepare failed: ' . $conn->error); + } + if (!empty($params)) { + $stmt->bind_param($types, ...$params); + } + $stmt->execute(); + $result = $stmt->get_result(); +} catch (Throwable $e) { + error_log('check_duplicates: ' . $e->getMessage()); + ResponseHelper::success(['duplicates' => []]); } -$stmt->execute(); -$result = $stmt->get_result(); while ($row = $result->fetch_assoc()) { // Calculate similarity score diff --git a/api/manage_workflows.php b/api/manage_workflows.php index 9dbeec4..6961d2f 100644 --- a/api/manage_workflows.php +++ b/api/manage_workflows.php @@ -82,6 +82,15 @@ try { case 'POST': $data = json_decode(file_get_contents('php://input'), true); + $wfValid = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed']; + if ( + !in_array($data['from_status'] ?? '', $wfValid, true) + || !in_array($data['to_status'] ?? '', $wfValid, true) + ) { + http_response_code(400); + echo json_encode(['success' => false, 'error' => 'from_status and to_status must be valid ticket statuses']); + exit; + } if (($data['from_status'] ?? '') === ($data['to_status'] ?? '')) { http_response_code(400); echo json_encode(['success' => false, 'error' => 'From Status and To Status cannot be the same']); @@ -125,6 +134,15 @@ try { $data = json_decode(file_get_contents('php://input'), true); + $wfValid = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed']; + if ( + !in_array($data['from_status'] ?? '', $wfValid, true) + || !in_array($data['to_status'] ?? '', $wfValid, true) + ) { + http_response_code(400); + echo json_encode(['success' => false, 'error' => 'from_status and to_status must be valid ticket statuses']); + exit; + } if (($data['from_status'] ?? '') === ($data['to_status'] ?? '')) { http_response_code(400); echo json_encode(['success' => false, 'error' => 'From Status and To Status cannot be the same']); diff --git a/create_ticket_api.php b/create_ticket_api.php index 24dd618..0e9e1e9 100644 --- a/create_ticket_api.php +++ b/create_ticket_api.php @@ -231,6 +231,22 @@ $priority = $data['priority'] ?? '4'; $category = (string)($data['category'] ?? 'General'); $type = (string)($data['type'] ?? 'Issue'); +// Validate externally-supplied status and priority. (category/type are free-form +// in this schema.) A non-numeric priority would otherwise cast to 0 and escalate +// the ticket below P1 on the dedup/update path. +$validStatuses = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed']; +if (!in_array($status, $validStatuses, true)) { + http_response_code(400); + echo json_encode(['success' => false, 'error' => 'Invalid status']); + exit; +} +if (!is_numeric($priority) || (int)$priority < 1 || (int)$priority > 5) { + http_response_code(400); + echo json_encode(['success' => false, 'error' => 'Invalid priority (must be 1-5)']); + exit; +} +$priority = (int)$priority; + $ticketHash = generateTicketHash($data); $auditLog = new AuditLogModel($conn); diff --git a/helpers/NotificationHelper.php b/helpers/NotificationHelper.php index 91cc16d..7bb1304 100644 --- a/helpers/NotificationHelper.php +++ b/helpers/NotificationHelper.php @@ -164,32 +164,38 @@ class NotificationHelper return; } - // Fetch watcher usernames, excluding the actor so they don't notify themselves - if ($excludeUserId !== null) { - $sql = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ? AND tw.user_id != ?"; - $stmt = $conn->prepare($sql); - } else { - $sql = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ?"; - $stmt = $conn->prepare($sql); - } - // Notifications are best-effort; if the watchers table is absent or the - // statement fails to prepare, skip silently rather than fataling the - // request that already committed its DB change. - if (!$stmt) { - return; - } - if ($excludeUserId !== null) { - $stmt->bind_param("ii", $ticketId, $excludeUserId); - } else { - $stmt->bind_param("i", $ticketId); - } - $stmt->execute(); - $result = $stmt->get_result(); - $stmt->close(); - + // Fetch watcher usernames, excluding the actor so they don't notify + // themselves. Notifications are best-effort: if the watchers table is + // absent or the query fails, skip silently rather than fataling the + // request that already committed its DB change. mysqli may either throw + // (default exception mode) or return false, so handle both. $usernames = []; - while ($row = $result->fetch_assoc()) { - $usernames[] = $row['username']; + try { + if ($excludeUserId !== null) { + $sql = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ? AND tw.user_id != ?"; + $stmt = $conn->prepare($sql); + } else { + $sql = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ?"; + $stmt = $conn->prepare($sql); + } + if (!$stmt) { + return; + } + if ($excludeUserId !== null) { + $stmt->bind_param("ii", $ticketId, $excludeUserId); + } else { + $stmt->bind_param("i", $ticketId); + } + $stmt->execute(); + $result = $stmt->get_result(); + $stmt->close(); + + while ($row = $result->fetch_assoc()) { + $usernames[] = $row['username']; + } + } catch (\Throwable $e) { + error_log('NotificationHelper::notifyWatchers watcher lookup failed: ' . $e->getMessage()); + return; } if (empty($usernames)) { diff --git a/models/TicketModel.php b/models/TicketModel.php index 23506c3..ae77308 100644 --- a/models/TicketModel.php +++ b/models/TicketModel.php @@ -208,6 +208,11 @@ class TicketModel ORDER BY $sortExpression $sortDirection LIMIT ? OFFSET ?"; + // Keep a copy of the filter params (without LIMIT/OFFSET) for the + // fallback COUNT below. + $countParams = $params; + $countParamTypes = $paramTypes; + $params[] = $limit; $params[] = $offset; $paramTypes .= 'ii'; @@ -228,6 +233,24 @@ class TicketModel } $stmt->close(); + // COUNT(*) OVER() rides on returned rows, so a page past the last row + // yields zero rows and a bogus total of 0. Fall back to a direct COUNT + // so the total/pages stay correct for stale or over-range page links. + if ($totalTickets === 0 && $offset > 0) { + $countSql = "SELECT COUNT(*) AS c + FROM tickets t + LEFT JOIN users u_created ON t.created_by = u_created.user_id + LEFT JOIN users u_assigned ON t.assigned_to = u_assigned.user_id + $whereClause"; + $countStmt = $this->conn->prepare($countSql); + if (!empty($countParams)) { + $countStmt->bind_param($countParamTypes, ...$countParams); + } + $countStmt->execute(); + $totalTickets = (int)($countStmt->get_result()->fetch_assoc()['c'] ?? 0); + $countStmt->close(); + } + return [ 'tickets' => $tickets, 'total' => $totalTickets, diff --git a/views/DashboardView.php b/views/DashboardView.php index 7dc5479..4384d98 100644 --- a/views/DashboardView.php +++ b/views/DashboardView.php @@ -48,14 +48,14 @@ if (!empty($_GET['priority'])) { } } if (!empty($_GET['category'])) { - $activeFilters[] = ['type' => 'category', 'value' => $_GET['category'], 'label' => 'Category: ' . htmlspecialchars($_GET['category'])]; + $activeFilters[] = ['type' => 'category', 'value' => $_GET['category'], 'label' => 'Category: ' . $_GET['category']]; } if (!empty($_GET['type'])) { - $activeFilters[] = ['type' => 'type', 'value' => $_GET['type'], 'label' => 'Type: ' . htmlspecialchars($_GET['type'])]; + $activeFilters[] = ['type' => 'type', 'value' => $_GET['type'], 'label' => 'Type: ' . $_GET['type']]; } if (!empty($_GET['assigned_to'])) { $label = match ($_GET['assigned_to']) { - 'unassigned' => 'Unassigned', 'me' => 'Me', default => 'User #' . htmlspecialchars($_GET['assigned_to']) + 'unassigned' => 'Unassigned', 'me' => 'Me', default => 'User #' . $_GET['assigned_to'] }; $activeFilters[] = ['type' => 'assigned_to', 'value' => $_GET['assigned_to'], 'label' => 'Assigned: ' . $label]; }