From 882ab2662c37b1122e508387e56fb28396598dd9 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 10:56:52 -0400 Subject: [PATCH 1/8] Fix data-layer bugs: bind_param fatals, ticket_id bindings, cache poisoning - CustomFieldModel: assign ?? fallbacks to variables before bind_param (by-reference args cannot be ?? expressions; fatal on PHP 8.2, custom fields were uncreatable/uneditable) - RecurringTicketModel::create: fix swapped bind type for schedule_type (enum bound as int coerced 'daily' to 0, breaking the cron) - TicketModel/CommentModel: bind varchar ticket_id as string not int so the unique index is usable and leading-zero IDs match; ticket_watchers (int column) left as integer - TicketModel::deleteTicket: delete from custom_field_values (real table) not the nonexistent ticket_custom_fields - TicketModel search: honor literal '0'; never emit AGAINST('*') on all-special-char input (fall back to LIKE) - TicketModel::updateTicket: disambiguate not-found vs no-op vs genuine optimistic-lock conflict on zero affected rows - WorkflowModel: do not cache transitions/statuses on DB failure (a transient error no longer blocks all status changes for the TTL) - DependencyModel: filter linked tickets by visibility (new optional user context params) to stop confidential metadata leaking via dependencies - BulkOperationsModel: validate status/priority/assignee before mutating - AuditLogModel: gate getClientIP forwarded headers on trusted proxies; add missing action/entity types so audit-log filters work - WorkflowModel: add transitionRequiresComment() accessor for enforcement - CommentModel: stop leaking raw DB errors to clients (log instead) Co-Authored-By: Claude Opus 4.8 --- models/AuditLogModel.php | 56 +++++++++++------ models/BulkOperationsModel.php | 66 ++++++++++++++++++++ models/CommentModel.php | 20 ++++--- models/CustomFieldModel.php | 20 +++++-- models/DependencyModel.php | 64 ++++++++++++++++++-- models/RecurringTicketModel.php | 2 +- models/TicketModel.php | 77 ++++++++++++++---------- models/WorkflowModel.php | 103 ++++++++++++++++++++------------ 8 files changed, 301 insertions(+), 107 deletions(-) diff --git a/models/AuditLogModel.php b/models/AuditLogModel.php index 243e32d..2da878d 100644 --- a/models/AuditLogModel.php +++ b/models/AuditLogModel.php @@ -19,13 +19,15 @@ class AuditLogModel /** @var array Allowed action types for filtering */ private const VALID_ACTION_TYPES = [ 'create', 'update', 'delete', 'view', 'security_event', - 'login', 'logout', 'assign', 'comment', 'bulk_update' + 'login', 'logout', 'assign', 'unassign', 'comment', 'mention', + 'revoke', 'attachment_upload', 'attachment_delete', 'bulk_update' ]; /** @var array Allowed entity types for filtering */ private const VALID_ENTITY_TYPES = [ 'ticket', 'comment', 'user', 'api_key', 'security', - 'template', 'attachment', 'group' + 'template', 'attachment', 'ticket_attachments', 'group', + 'dependency', 'workflow_transition' ]; public function __construct($conn) @@ -327,24 +329,44 @@ class AuditLogModel */ private function getClientIP() { - $ipAddress = ''; + $remoteAddr = $_SERVER['REMOTE_ADDR'] ?? ''; - // Check for proxy headers - if (!empty($_SERVER['HTTP_CF_CONNECTING_IP'])) { - // Cloudflare - $ipAddress = $_SERVER['HTTP_CF_CONNECTING_IP']; - } elseif (!empty($_SERVER['HTTP_X_REAL_IP'])) { - // Nginx proxy - $ipAddress = $_SERVER['HTTP_X_REAL_IP']; - } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { - // Standard proxy header - $ipAddress = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])[0]; - } elseif (!empty($_SERVER['REMOTE_ADDR'])) { - // Direct connection - $ipAddress = $_SERVER['REMOTE_ADDR']; + // Forwarded/proxy headers are client-controlled, so only believe them when + // the request actually came from a trusted reverse proxy (same rule as + // RateLimitMiddleware). Otherwise a client could forge its audit-log IP. + $trusted = $GLOBALS['config']['TRUSTED_PROXIES'] ?? []; + if (empty($trusted) || !in_array($remoteAddr, $trusted, true)) { + return trim($remoteAddr); } - return trim($ipAddress); + // Cloudflare sets CF-Connecting-IP to the real client. + if ( + !empty($_SERVER['HTTP_CF_CONNECTING_IP']) + && filter_var($_SERVER['HTTP_CF_CONNECTING_IP'], FILTER_VALIDATE_IP) + ) { + return trim($_SERVER['HTTP_CF_CONNECTING_IP']); + } + + // The trusted proxy appends the connecting client to X-Forwarded-For, so + // the RIGHTMOST entry is the IP it observed (any client-supplied prefix is + // not trustworthy). + if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { + $ips = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']); + $ip = trim(end($ips)); + if (filter_var($ip, FILTER_VALIDATE_IP)) { + return $ip; + } + } + + // X-Real-IP is set by the proxy itself. + if ( + !empty($_SERVER['HTTP_X_REAL_IP']) + && filter_var($_SERVER['HTTP_X_REAL_IP'], FILTER_VALIDATE_IP) + ) { + return trim($_SERVER['HTTP_X_REAL_IP']); + } + + return trim($remoteAddr); } /** diff --git a/models/BulkOperationsModel.php b/models/BulkOperationsModel.php index 2305670..713e28f 100644 --- a/models/BulkOperationsModel.php +++ b/models/BulkOperationsModel.php @@ -77,6 +77,15 @@ class BulkOperationsModel $ticketIds = explode(',', $operation['ticket_ids']); $parameters = $operation['parameters'] ? json_decode($operation['parameters'], true) : []; + + // Validate operation parameters up front so invalid values (out-of-range + // priority, unknown status, nonexistent assignee) are rejected cleanly + // instead of corrupting tickets or throwing mid-transaction. + $paramError = $this->validateOperationParameters($operation['operation_type'], is_array($parameters) ? $parameters : []); + if ($paramError !== null) { + return ['processed' => 0, 'failed' => count($ticketIds), 'error' => $paramError]; + } + $processed = 0; $failed = 0; $errors = []; @@ -297,6 +306,63 @@ class BulkOperationsModel return $result; } + /** + * Validate the parameters for a bulk operation before any ticket is mutated. + * + * @return string|null Error message, or null if the parameters are valid + */ + private function validateOperationParameters(string $type, array $parameters): ?string + { + switch ($type) { + case 'bulk_priority': + if (!isset($parameters['priority'])) { + return 'Missing priority parameter'; + } + $priority = $parameters['priority']; + // tickets.priority has a CHECK constraint (between 1 and 6). + if (!is_numeric($priority) || (int)$priority < 1 || (int)$priority > 6) { + return 'Invalid priority: must be between 1 and 6'; + } + break; + + case 'bulk_status': + if (!isset($parameters['status'])) { + return 'Missing status parameter'; + } + $validStatuses = $GLOBALS['config']['TICKET_STATUSES'] + ?? ['Open', 'Pending', 'In Progress', 'Closed']; + if (!in_array($parameters['status'], $validStatuses, true)) { + return 'Invalid status value'; + } + break; + + case 'bulk_assign': + if (!isset($parameters['assigned_to'])) { + return 'Missing assigned_to parameter'; + } + $assignedTo = $parameters['assigned_to']; + if (!is_numeric($assignedTo) || (int)$assignedTo <= 0 || !$this->userExists((int)$assignedTo)) { + return 'Invalid assigned_to: user does not exist'; + } + break; + } + + return null; + } + + /** + * Check whether a user ID exists. + */ + private function userExists(int $userId): bool + { + $stmt = $this->conn->prepare("SELECT 1 FROM users WHERE user_id = ? LIMIT 1"); + $stmt->bind_param("i", $userId); + $stmt->execute(); + $exists = $stmt->get_result()->num_rows > 0; + $stmt->close(); + return $exists; + } + /** * Get bulk operation by ID * diff --git a/models/CommentModel.php b/models/CommentModel.php index 50f609a..419f26b 100644 --- a/models/CommentModel.php +++ b/models/CommentModel.php @@ -58,12 +58,12 @@ class CommentModel /** * Get total comment count for a ticket */ - public function getCommentCount(int $ticketId): int + public function getCommentCount(string $ticketId): int { $stmt = $this->conn->prepare( "SELECT COUNT(*) as total FROM ticket_comments WHERE ticket_id = ?" ); - $stmt->bind_param("i", $ticketId); + $stmt->bind_param("s", $ticketId); $stmt->execute(); $row = $stmt->get_result()->fetch_assoc(); $stmt->close(); @@ -108,9 +108,9 @@ class CommentModel $stmt = $this->conn->prepare($sql); if ($limit > 0) { - $stmt->bind_param("iii", $ticketId, $limit, $offset); + $stmt->bind_param("sii", $ticketId, $limit, $offset); } else { - $stmt->bind_param("i", $ticketId); + $stmt->bind_param("s", $ticketId); } $stmt->execute(); $result = $stmt->get_result(); @@ -146,7 +146,7 @@ class CommentModel /** * Paginated threaded comments: fetch one page of root comments + all their replies. */ - private function getThreadedCommentsPaged(int $ticketId, int $limit, int $offset): array + private function getThreadedCommentsPaged(string $ticketId, int $limit, int $offset): array { // Page of root comments $rootSql = "SELECT tc.*, u.display_name, u.username @@ -156,7 +156,7 @@ class CommentModel ORDER BY tc.created_at DESC LIMIT ? OFFSET ?"; $stmt = $this->conn->prepare($rootSql); - $stmt->bind_param("iii", $ticketId, $limit, $offset); + $stmt->bind_param("sii", $ticketId, $limit, $offset); $stmt->execute(); $rootResult = $stmt->get_result(); $stmt->close(); @@ -192,7 +192,7 @@ class CommentModel AND tc.parent_comment_id IN ($placeholders) ORDER BY tc.created_at ASC"; $replyStmt = $this->conn->prepare($replySql); - $types = 'i' . str_repeat('i', count($parentIds)); + $types = 's' . str_repeat('i', count($parentIds)); $replyStmt->bind_param($types, $ticketId, ...$parentIds); $replyStmt->execute(); $replyResult = $replyStmt->get_result(); @@ -394,7 +394,8 @@ class CommentModel 'updated_at' => $hasUpdatedAt ? date('M d, Y H:i') : null ]; } else { - return ['success' => false, 'error' => $this->conn->error]; + error_log('CommentModel::updateComment failed: ' . $this->conn->error); + return ['success' => false, 'error' => 'Failed to update comment']; } } @@ -428,7 +429,8 @@ class CommentModel 'ticket_id' => $ticketId ]; } else { - return ['success' => false, 'error' => $this->conn->error]; + error_log('CommentModel::deleteComment failed: ' . $this->conn->error); + return ['success' => false, 'error' => 'Failed to delete comment']; } } } diff --git a/models/CustomFieldModel.php b/models/CustomFieldModel.php index d4af64d..4ebb3c0 100644 --- a/models/CustomFieldModel.php +++ b/models/CustomFieldModel.php @@ -96,6 +96,10 @@ class CustomFieldModel (field_name, field_label, field_type, field_options, category, is_required, display_order, is_active) VALUES (?, ?, ?, ?, ?, ?, ?, ?)"; + $isRequired = $data['is_required'] ?? 0; + $displayOrder = $data['display_order'] ?? 0; + $isActive = $data['is_active'] ?? 1; + $stmt = $this->conn->prepare($sql); $stmt->bind_param( 'sssssiii', @@ -104,9 +108,9 @@ class CustomFieldModel $data['field_type'], $options, $data['category'], - $data['is_required'] ?? 0, - $data['display_order'] ?? 0, - $data['is_active'] ?? 1 + $isRequired, + $displayOrder, + $isActive ); if ($stmt->execute()) { @@ -135,6 +139,10 @@ class CustomFieldModel category = ?, is_required = ?, display_order = ?, is_active = ? WHERE field_id = ?"; + $isRequired = $data['is_required'] ?? 0; + $displayOrder = $data['display_order'] ?? 0; + $isActive = $data['is_active'] ?? 1; + $stmt = $this->conn->prepare($sql); $stmt->bind_param( 'sssssiiii', @@ -143,9 +151,9 @@ class CustomFieldModel $data['field_type'], $options, $data['category'], - $data['is_required'] ?? 0, - $data['display_order'] ?? 0, - $data['is_active'] ?? 1, + $isRequired, + $displayOrder, + $isActive, $fieldId ); diff --git a/models/DependencyModel.php b/models/DependencyModel.php index ad5e85f..a7caf31 100644 --- a/models/DependencyModel.php +++ b/models/DependencyModel.php @@ -12,25 +12,67 @@ class DependencyModel $this->conn = $conn; } + /** + * Build the extra WHERE fragment (and bound params) that restricts the joined + * ticket alias `t` to tickets the requesting user may see. Reuses + * TicketModel::getVisibilityFilter so the rules stay in one place. + * + * @return array{sql:string,types:string,params:array} + */ + private function buildVisibilityClause($userId, array $userGroups, $isAdmin): array + { + if ($isAdmin) { + return ['sql' => '', 'types' => '', 'params' => []]; + } + + require_once dirname(__DIR__) . '/models/TicketModel.php'; + $ticketModel = new TicketModel($this->conn); + $filter = $ticketModel->getVisibilityFilter([ + 'user_id' => (int)$userId, + 'groups' => implode(',', $userGroups), + 'is_admin' => false, + ]); + + if ($filter['sql'] === '1=1' || $filter['sql'] === '') { + return ['sql' => '', 'types' => '', 'params' => []]; + } + + return [ + 'sql' => ' AND ' . $filter['sql'], + 'types' => $filter['types'], + 'params' => $filter['params'], + ]; + } + /** * Get all dependencies for a ticket * + * The linked ticket's title/status/priority are only returned for tickets the + * requesting user is allowed to see (same rules as TicketModel::getVisibilityFilter). + * With the default (null user, non-admin) only public tickets are exposed. + * * @param string $ticketId Ticket ID + * @param int|null $userId Requesting user's ID (null = anonymous) + * @param array $userGroups Requesting user's group names + * @param bool $isAdmin Whether the requesting user is an admin (bypasses filtering) * @return array Dependencies grouped by type */ - public function getDependencies($ticketId) + public function getDependencies($ticketId, $userId = null, array $userGroups = [], $isAdmin = false) { + $visibility = $this->buildVisibilityClause($userId, $userGroups, $isAdmin); + $sql = "SELECT d.*, t.title, t.status, t.priority FROM ticket_dependencies d LEFT JOIN tickets t ON d.depends_on_id = t.ticket_id - WHERE d.ticket_id = ? + WHERE d.ticket_id = ?" . $visibility['sql'] . " ORDER BY d.dependency_type, d.created_at DESC"; $stmt = $this->conn->prepare($sql); if (!$stmt) { throw new Exception('Prepare failed: ' . $this->conn->error); } - $stmt->bind_param("s", $ticketId); + $types = 's' . $visibility['types']; + $stmt->bind_param($types, $ticketId, ...$visibility['params']); if (!$stmt->execute()) { throw new Exception('Execute failed: ' . $stmt->error); } @@ -54,22 +96,32 @@ class DependencyModel /** * Get tickets that depend on this ticket * + * The linked ticket's title/status/priority are only returned for tickets the + * requesting user is allowed to see (same rules as TicketModel::getVisibilityFilter). + * With the default (null user, non-admin) only public tickets are exposed. + * * @param string $ticketId Ticket ID + * @param int|null $userId Requesting user's ID (null = anonymous) + * @param array $userGroups Requesting user's group names + * @param bool $isAdmin Whether the requesting user is an admin (bypasses filtering) * @return array Dependent tickets */ - public function getDependentTickets($ticketId) + public function getDependentTickets($ticketId, $userId = null, array $userGroups = [], $isAdmin = false) { + $visibility = $this->buildVisibilityClause($userId, $userGroups, $isAdmin); + $sql = "SELECT d.*, t.title, t.status, t.priority FROM ticket_dependencies d LEFT JOIN tickets t ON d.ticket_id = t.ticket_id - WHERE d.depends_on_id = ? + WHERE d.depends_on_id = ?" . $visibility['sql'] . " ORDER BY d.dependency_type, d.created_at DESC"; $stmt = $this->conn->prepare($sql); if (!$stmt) { throw new Exception('Prepare failed: ' . $this->conn->error); } - $stmt->bind_param("s", $ticketId); + $types = 's' . $visibility['types']; + $stmt->bind_param($types, $ticketId, ...$visibility['params']); if (!$stmt->execute()) { throw new Exception('Execute failed: ' . $stmt->error); } diff --git a/models/RecurringTicketModel.php b/models/RecurringTicketModel.php index 2e54b38..500a14f 100644 --- a/models/RecurringTicketModel.php +++ b/models/RecurringTicketModel.php @@ -65,7 +65,7 @@ class RecurringTicketModel $stmt = $this->conn->prepare($sql); $stmt->bind_param( - 'ssssiiisssii', + 'ssssiissssii', $data['title_template'], $data['description_template'], $data['category'], diff --git a/models/TicketModel.php b/models/TicketModel.php index ae77308..b64c837 100644 --- a/models/TicketModel.php +++ b/models/TicketModel.php @@ -9,7 +9,7 @@ class TicketModel $this->conn = $conn; } - public function getTicketById(int $id): ?array + public function getTicketById(string $id): ?array { $sql = "SELECT t.*, u_created.username as creator_username, @@ -24,7 +24,7 @@ class TicketModel LEFT JOIN users u_assigned ON t.assigned_to = u_assigned.user_id WHERE t.ticket_id = ?"; $stmt = $this->conn->prepare($sql); - $stmt->bind_param("i", $id); + $stmt->bind_param("s", $id); $stmt->execute(); $result = $stmt->get_result(); @@ -82,18 +82,22 @@ class TicketModel $paramTypes .= str_repeat('s', count($types)); } - // Search Functionality — use FULLTEXT when available, fall back to LIKE - if ($search && !empty($search)) { - if ($this->hasFulltextIndex()) { + // Search Functionality — use FULLTEXT when available, fall back to LIKE. + // Use a strict emptiness check so a literal "0" search is honored. + if ($search !== null && $search !== '') { + // Strip MySQL boolean mode special chars to prevent parse errors on user input + $ftSearch = trim(preg_replace('/\s+/', ' ', preg_replace('/[+\-><()\~*"@]+/', ' ', $search))); + if ($this->hasFulltextIndex() && $ftSearch !== '') { // MATCH...AGAINST for indexed full-text search (much faster at scale) - // Strip MySQL boolean mode special chars to prevent parse errors on user input - $ftSearch = preg_replace('/[+\-><()\~*"@]+/', ' ', $search); - $ftSearch = trim(preg_replace('/\s+/', ' ', $ftSearch)) . '*'; + $ftSearch .= '*'; $whereConditions[] = "(MATCH(t.title, t.description) AGAINST (? IN BOOLEAN MODE) OR t.ticket_id LIKE ? OR t.category LIKE ? OR t.type LIKE ?)"; $searchTerm = "%$search%"; $params = array_merge($params, [$ftSearch, $searchTerm, $searchTerm, $searchTerm]); $paramTypes .= 'ssss'; } else { + // No FULLTEXT index, or the sanitized boolean query is empty (search was + // only special chars) — fall back to LIKE instead of emitting invalid + // AGAINST('*' ...) syntax. $whereConditions[] = "(t.title LIKE ? OR t.description LIKE ? OR t.ticket_id LIKE ? OR t.category LIKE ? OR t.type LIKE ?)"; $searchTerm = "%$search%"; $params = array_merge($params, [$searchTerm, $searchTerm, $searchTerm, $searchTerm, $searchTerm]); @@ -308,7 +312,7 @@ class TicketModel if ($expectedUpdatedAt !== null) { $stmt->bind_param( - "sissssisis", + "sissssisss", $ticketData['title'], $ticketData['priority'], $ticketData['status'], @@ -322,7 +326,7 @@ class TicketModel ); } else { $stmt->bind_param( - "sissssisi", + "sissssiss", $ticketData['title'], $ticketData['priority'], $ticketData['status'], @@ -343,20 +347,31 @@ class TicketModel return ['success' => false, 'error' => 'Database error: ' . $this->conn->error, 'conflict' => false]; } - // Check for optimistic locking conflict - if ($expectedUpdatedAt !== null && $affectedRows === 0) { - // Either ticket doesn't exist or was modified by someone else + // Zero affected rows is ambiguous: the ticket may not exist, an optimistic + // lock may have failed, or the row simply matched with no column changes + // (identical resubmit). Disambiguate so we neither report a false conflict + // nor silently "succeed" on a non-existent ticket. + if ($affectedRows === 0) { $ticket = $this->getTicketById($ticketData['ticket_id']); - if ($ticket) { - return [ - 'success' => false, - 'error' => 'This ticket was modified by another user. Please refresh and try again.', - 'conflict' => true, - 'current_updated_at' => $ticket['updated_at'] - ]; - } else { + if (!$ticket) { return ['success' => false, 'error' => 'Ticket not found', 'conflict' => false]; } + + if ($expectedUpdatedAt !== null) { + // Only a genuine concurrent modification changes updated_at. If it + // still equals the expected value the WHERE matched but nothing + // changed (e.g. identical data resubmitted within the same second), + // which is not a conflict. + if ($ticket['updated_at'] !== $expectedUpdatedAt) { + return [ + 'success' => false, + 'error' => 'This ticket was modified by another user. Please refresh and try again.', + 'conflict' => true, + 'current_updated_at' => $ticket['updated_at'] + ]; + } + } + // Ticket exists and no conflict: treat no-op update as success. } return ['success' => true, 'error' => null, 'conflict' => false]; @@ -516,9 +531,9 @@ class TicketModel } } - public function addComment(int $ticketId, array $commentData): array + public function addComment(string $ticketId, array $commentData): array { - $sql = "INSERT INTO ticket_comments (ticket_id, user_name, comment_text, markdown_enabled) + $sql = "INSERT INTO ticket_comments (ticket_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, ?, ?)"; $stmt = $this->conn->prepare($sql); @@ -528,7 +543,7 @@ class TicketModel $markdownEnabled = $commentData['markdown_enabled'] ? 1 : 0; $stmt->bind_param( - "issi", + "sssi", $ticketId, $username, $commentData['comment_text'], @@ -557,11 +572,11 @@ class TicketModel * @param int $assignedBy User ID performing the assignment * @return bool Success status */ - public function assignTicket(int $ticketId, int $userId, int $assignedBy): bool + public function assignTicket(string $ticketId, int $userId, int $assignedBy): bool { $sql = "UPDATE tickets SET assigned_to = ?, updated_by = ?, updated_at = NOW() WHERE ticket_id = ?"; $stmt = $this->conn->prepare($sql); - $stmt->bind_param("iii", $userId, $assignedBy, $ticketId); + $stmt->bind_param("iis", $userId, $assignedBy, $ticketId); $result = $stmt->execute(); $stmt->close(); return $result; @@ -574,11 +589,11 @@ class TicketModel * @param int $updatedBy User ID performing the unassignment * @return bool Success status */ - public function unassignTicket(int $ticketId, int $updatedBy): bool + public function unassignTicket(string $ticketId, int $updatedBy): bool { $sql = "UPDATE tickets SET assigned_to = NULL, updated_by = ?, updated_at = NOW() WHERE ticket_id = ?"; $stmt = $this->conn->prepare($sql); - $stmt->bind_param("ii", $updatedBy, $ticketId); + $stmt->bind_param("is", $updatedBy, $ticketId); $result = $stmt->execute(); $stmt->close(); return $result; @@ -733,7 +748,7 @@ class TicketModel * @param int $updatedBy User ID * @return bool */ - public function updateVisibility(int $ticketId, string $visibility, ?string $visibilityGroups, int $updatedBy): bool + public function updateVisibility(string $ticketId, string $visibility, ?string $visibilityGroups, int $updatedBy): bool { $allowedVisibilities = ['public', 'internal', 'confidential']; if (!in_array($visibility, $allowedVisibilities)) { @@ -752,7 +767,7 @@ class TicketModel $sql = "UPDATE tickets SET visibility = ?, visibility_groups = ?, updated_by = ?, updated_at = NOW() WHERE ticket_id = ?"; $stmt = $this->conn->prepare($sql); - $stmt->bind_param("ssii", $visibility, $visibilityGroups, $updatedBy, $ticketId); + $stmt->bind_param("ssis", $visibility, $visibilityGroups, $updatedBy, $ticketId); $result = $stmt->execute(); $stmt->close(); return $result; @@ -790,7 +805,7 @@ class TicketModel "DELETE FROM ticket_watchers WHERE ticket_id = ?", "DELETE FROM ticket_dependencies WHERE ticket_id = ? OR depends_on_id = ?", "DELETE FROM ticket_attachments WHERE ticket_id = ?", - "DELETE FROM ticket_custom_fields WHERE ticket_id = ?", + "DELETE FROM custom_field_values WHERE ticket_id = ?", ]; foreach ($children as $sql) { diff --git a/models/WorkflowModel.php b/models/WorkflowModel.php index 3906eff..473bfed 100644 --- a/models/WorkflowModel.php +++ b/models/WorkflowModel.php @@ -26,31 +26,38 @@ class WorkflowModel */ private function getAllTransitions(): array { - return CacheHelper::remember(self::$CACHE_PREFIX, 'all_transitions', function () { - $sql = "SELECT from_status, to_status, requires_comment, requires_admin - FROM status_transitions - WHERE is_active = TRUE"; - $result = $this->conn->query($sql); + $cached = CacheHelper::get(self::$CACHE_PREFIX, 'all_transitions', self::$CACHE_TTL); + if ($cached !== null) { + return $cached; + } - if (!$result) { - return []; + $sql = "SELECT from_status, to_status, requires_comment, requires_admin + FROM status_transitions + WHERE is_active = TRUE"; + $result = $this->conn->query($sql); + + if (!$result) { + // A transient DB failure must NOT be cached as "no transitions" — that + // would block every status change for the whole TTL. Fail safe by + // returning empty without storing it, so the next call retries. + return []; + } + + $transitions = []; + while ($row = $result->fetch_assoc()) { + $from = $row['from_status']; + if (!isset($transitions[$from])) { + $transitions[$from] = []; } + $transitions[$from][$row['to_status']] = [ + 'to_status' => $row['to_status'], + 'requires_comment' => (bool)$row['requires_comment'], + 'requires_admin' => (bool)$row['requires_admin'] + ]; + } - $transitions = []; - while ($row = $result->fetch_assoc()) { - $from = $row['from_status']; - if (!isset($transitions[$from])) { - $transitions[$from] = []; - } - $transitions[$from][$row['to_status']] = [ - 'to_status' => $row['to_status'], - 'requires_comment' => (bool)$row['requires_comment'], - 'requires_admin' => (bool)$row['requires_admin'] - ]; - } - - return $transitions; - }, self::$CACHE_TTL); + CacheHelper::set(self::$CACHE_PREFIX, 'all_transitions', $transitions); + return $transitions; } /** @@ -107,24 +114,29 @@ class WorkflowModel */ public function getAllStatuses(): array { - return CacheHelper::remember(self::$CACHE_PREFIX, 'all_statuses', function () { - $sql = "SELECT DISTINCT from_status as status FROM status_transitions - UNION - SELECT DISTINCT to_status as status FROM status_transitions - ORDER BY status"; - $result = $this->conn->query($sql); + $cached = CacheHelper::get(self::$CACHE_PREFIX, 'all_statuses', self::$CACHE_TTL); + if ($cached !== null) { + return $cached; + } - if (!$result) { - return []; - } + $sql = "SELECT DISTINCT from_status as status FROM status_transitions + UNION + SELECT DISTINCT to_status as status FROM status_transitions + ORDER BY status"; + $result = $this->conn->query($sql); - $statuses = []; - while ($row = $result->fetch_assoc()) { - $statuses[] = $row['status']; - } + if (!$result) { + // Do not cache an empty list on a transient DB failure. + return []; + } - return $statuses; - }, self::$CACHE_TTL); + $statuses = []; + while ($row = $result->fetch_assoc()) { + $statuses[] = $row['status']; + } + + CacheHelper::set(self::$CACHE_PREFIX, 'all_statuses', $statuses); + return $statuses; } /** @@ -149,6 +161,23 @@ class WorkflowModel ]; } + /** + * Whether a given transition requires a comment. + * + * Convenience accessor so callers (e.g. the update-ticket endpoint) can + * enforce requires_comment server-side without inspecting the full row. + * Returns false for an undefined transition or a no-op (same status). + * + * @param string $fromStatus Current status + * @param string $toStatus Desired status + * @return bool True if the transition requires a comment + */ + public function transitionRequiresComment(string $fromStatus, string $toStatus): bool + { + $requirements = $this->getTransitionRequirements($fromStatus, $toStatus); + return $requirements !== null && !empty($requirements['requires_comment']); + } + /** * Clear workflow cache (call when transitions are modified) */ From c5f7a01e1d1a771316c16d5d476c0b4a506df95e Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 11:17:07 -0400 Subject: [PATCH 2/8] Fix helpers/config: timezone, comment leak, silent misconfig, cache perms - Database.php: pin MySQL session time_zone to the configured named zone (mysql.time_zone tables now loaded on the DB) with a fixed-offset fallback, so NOW()/TIMESTAMP and PHP agree regardless of the DB server's SYSTEM tz. Best-effort, never fatals the connection. - NotificationHelper: redact comment-body previews for internal/ confidential tickets in sendCommentNotification and notifyWatchers so they are not leaked to the shared Matrix notify list (new $visibility param; callers wired in the API batch). - config.php: die with a clear error if parse_ini_file fails instead of silently falling back to insecure defaults (empty DB pass / proxies). - CacheHelper: create cache dir 0700 and cache files 0600 so other local users cannot read or poison security-relevant cached data. Co-Authored-By: Claude Opus 4.8 --- config/config.php | 3 +++ helpers/CacheHelper.php | 16 ++++++++++++++-- helpers/Database.php | 26 ++++++++++++++++++++++++++ helpers/NotificationHelper.php | 30 ++++++++++++++++++++++++++---- 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/config/config.php b/config/config.php index e25b5a9..3ffb242 100644 --- a/config/config.php +++ b/config/config.php @@ -6,6 +6,9 @@ if (!file_exists($envFile)) { die('Configuration error: .env file not found. Copy .env.example to .env and configure your database settings.'); } $envVars = parse_ini_file($envFile, false, INI_SCANNER_TYPED); +if (!is_array($envVars)) { + die('Configuration error: .env file could not be parsed. Check for unquoted special characters (e.g. #, ;, =, or quotes) in values and wrap affected values in double quotes.'); +} // Strip quotes from values if present (parse_ini_file may include them) if ($envVars) { diff --git a/helpers/CacheHelper.php b/helpers/CacheHelper.php index 0e5f2e5..40c1c83 100644 --- a/helpers/CacheHelper.php +++ b/helpers/CacheHelper.php @@ -21,7 +21,13 @@ class CacheHelper if (self::$cacheDir === null) { self::$cacheDir = sys_get_temp_dir() . '/tinker_tickets_cache'; if (!is_dir(self::$cacheDir)) { - mkdir(self::$cacheDir, 0755, true); + // 0700: only the app user may read cached data or create files. + // mkdir mode is masked by umask, so chmod to enforce it. + mkdir(self::$cacheDir, 0700, true); + @chmod(self::$cacheDir, 0700); + } elseif (!function_exists('posix_geteuid') || fileowner(self::$cacheDir) === posix_geteuid()) { + // Existing dir we own: harden a previously world-readable dir. + @chmod(self::$cacheDir, 0700); } } return self::$cacheDir; @@ -106,7 +112,13 @@ class CacheHelper // Store in file cache $filePath = self::getCacheDir() . '/' . $key . '.json'; - return @file_put_contents($filePath, json_encode($cached), LOCK_EX) !== false; + $written = @file_put_contents($filePath, json_encode($cached), LOCK_EX) !== false; + if ($written) { + // 0600: cache may feed security-relevant reads; keep it non-readable + // to other local users and non-poisonable by pre-created files. + @chmod($filePath, 0600); + } + return $written; } /** diff --git a/helpers/Database.php b/helpers/Database.php index 7b75de9..b6d2f7a 100644 --- a/helpers/Database.php +++ b/helpers/Database.php @@ -57,6 +57,32 @@ class Database // Set charset to utf8mb4 for proper Unicode support $conn->set_charset('utf8mb4'); + // Pin the MySQL session time zone to the app's configured zone so that + // NOW()/CURRENT_TIMESTAMP and PHP agree on wall-clock time regardless of + // the DB server's SYSTEM tz. Prefer the named zone (requires the + // mysql.time_zone_* tables); if that isn't available, fall back to the + // fixed numeric offset PHP computes for the same zone. Best-effort: a + // failure here must never fatal the connection. + $tz = $GLOBALS['config']['TIMEZONE'] ?? 'UTC'; + try { + $escaped = $conn->real_escape_string($tz); + try { + // mysqli throws (does not return false) on failure under the + // default PHP 8.1+ report mode, so catch it rather than testing + // the return value. + $conn->query("SET time_zone = '{$escaped}'"); + } catch (\Throwable $inner) { + // Named zone unavailable (mysql.time_zone_* not populated) — fall + // back to a fixed numeric offset so PHP and MySQL still agree on + // wall-clock time regardless of the DB server's SYSTEM tz. + $offset = (new DateTime('now', new DateTimeZone($tz)))->format('P'); + $escapedOffset = $conn->real_escape_string($offset); + $conn->query("SET time_zone = '{$escapedOffset}'"); + } + } catch (\Throwable $e) { + error_log('Database: failed to set session time_zone: ' . $e->getMessage()); + } + return $conn; } diff --git a/helpers/NotificationHelper.php b/helpers/NotificationHelper.php index 7bb1304..1afceac 100644 --- a/helpers/NotificationHelper.php +++ b/helpers/NotificationHelper.php @@ -96,21 +96,31 @@ class NotificationHelper * @param string $commentText Plain text (first 200 chars will be sent) * @param string|null $authorDisplay Display name of commenter * @param bool $isInternal True if the comment is internal-only + * @param string $visibility Ticket visibility: 'public', 'internal', or + * 'confidential'. For non-public tickets the + * comment text preview is redacted so it is + * never leaked to the shared notify list. */ - public static function sendCommentNotification($ticketId, string $ticketTitle, string $commentText, ?string $authorDisplay = null, bool $isInternal = false): void + public static function sendCommentNotification($ticketId, string $ticketTitle, string $commentText, ?string $authorDisplay = null, bool $isInternal = false, string $visibility = 'public'): void { - // Skip if this is an internal-only comment — only the assignee/admin need to know $notifyUsers = self::notifyUsers(); if (empty($notifyUsers)) { return; } + // The shared notify list may include users without access to non-public + // tickets, so never post the comment body for internal/confidential + // tickets — only that activity occurred. + $preview = $visibility === 'public' + ? mb_strimwidth($commentText, 0, 200, '…') + : null; + self::fire([ 'event' => 'comment_added', 'ticket_id' => $ticketId, 'title' => $ticketTitle, 'author' => $authorDisplay, - 'preview' => mb_strimwidth($commentText, 0, 200, '…'), + 'preview' => $preview, 'is_internal' => $isInternal, 'url' => UrlHelper::ticketUrl($ticketId), 'notify_users' => $notifyUsers, @@ -155,8 +165,14 @@ class NotificationHelper * @param string $event One of: status_changed, comment_added, assigned * @param array $extraData Merged into the payload (old_status/new_status, author, etc.) * @param int|null $excludeUserId Don't notify the actor themselves + * @param string $visibility Ticket visibility: 'public', 'internal', or + * 'confidential'. notify_users includes the + * shared list, which may contain users without + * access to non-public tickets, so any comment + * body preview in $extraData is redacted for + * non-public tickets. */ - public static function notifyWatchers(\mysqli $conn, $ticketId, string $ticketTitle, string $event, array $extraData = [], ?int $excludeUserId = null): void + public static function notifyWatchers(\mysqli $conn, $ticketId, string $ticketTitle, string $event, array $extraData = [], ?int $excludeUserId = null, string $visibility = 'public'): void { $webhookUrl = $GLOBALS['config']['MATRIX_WEBHOOK_URL'] ?? null; $domain = $GLOBALS['config']['MATRIX_DOMAIN'] ?? null; @@ -164,6 +180,12 @@ class NotificationHelper return; } + // Don't leak comment/body content to the shared notify list for + // non-public tickets — keep only the fact that activity occurred. + if ($visibility !== 'public' && isset($extraData['preview'])) { + $extraData['preview'] = null; + } + // 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 From 327c225ded4bdc15cbcf5d436ba5e5f6f435d219 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 11:48:34 -0400 Subject: [PATCH 3/8] Fix API security: dependency/visibility leaks, authz, CSRF, comment spoofing - ticket_dependencies.php: pass current user id/groups/is_admin into the visibility-filtered DependencyModel methods; drop (int) casts that stripped leading zeros from varchar ticket_ids - update_ticket.php: authorize visibility changes (admin or creator only); enforce requires_comment transitions server-side (400 + requires_comment flag so the client can prompt-and-retry); return proper 401/400/403 - add_comment.php: take commenter name from the session not the client (anti-spoofing); validate parent_comment_id belongs to the ticket; reject empty comments; pass ticket visibility to notifications so non-public comment bodies aren't leaked - add_comment/update_comment/bulk_operation: validate CSRF for all state-changing methods, not just POST - bootstrap.php: return the current CSRF token on rejection and never rotate it on a rejected request, so a desynced client can auto-recover - correct auth->401 and validation->400 status codes across these endpoints Co-Authored-By: Claude Opus 4.8 --- api/add_comment.php | 55 ++++++++++++++++++++++++++++++---- api/bootstrap.php | 9 +++++- api/bulk_operation.php | 6 ++-- api/delete_comment.php | 12 ++++++-- api/ticket_dependencies.php | 17 ++++++----- api/update_comment.php | 22 ++++++++++---- api/update_ticket.php | 60 +++++++++++++++++++++++++++++++++---- 7 files changed, 152 insertions(+), 29 deletions(-) diff --git a/api/add_comment.php b/api/add_comment.php index 0b13d40..d78be47 100644 --- a/api/add_comment.php +++ b/api/add_comment.php @@ -38,12 +38,16 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { - throw new Exception("Authentication required"); + ob_end_clean(); + http_response_code(401); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Authentication required']); + exit; } - // CSRF Protection + // CSRF Protection for all state-changing methods (any non-GET/HEAD request) require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php'; - if ($_SERVER['REQUEST_METHOD'] === 'POST') { + if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) { $csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? ''; if (!CsrfMiddleware::validateToken($csrfToken)) { http_response_code(403); @@ -63,7 +67,11 @@ try { $data = json_decode(file_get_contents('php://input'), true); if (!$data) { - throw new Exception("Invalid JSON data received"); + http_response_code(400); + ob_end_clean(); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Invalid JSON data received']); + exit; } $ticketId = isset($data['ticket_id']) ? trim((string)$data['ticket_id']) : ''; @@ -75,6 +83,20 @@ try { exit; } + // Reject empty/whitespace-only comments + $commentTextRaw = isset($data['comment_text']) ? trim((string)$data['comment_text']) : ''; + if ($commentTextRaw === '') { + http_response_code(400); + ob_end_clean(); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Comment text cannot be empty']); + exit; + } + + // Never trust a client-supplied display name — always attribute the comment to + // the authenticated session user. + $data['user_name'] = $currentUser['display_name'] ?? $currentUser['username'] ?? 'User'; + // Verify user can access the ticket before allowing a comment $ticketModel = new TicketModel($conn); $ticket = $ticketModel->getTicketById($ticketId); @@ -97,6 +119,18 @@ try { $commentModel = new CommentModel($conn); $auditLog = new AuditLogModel($conn); + // If replying, the parent comment must belong to this same (accessible) ticket. + if (isset($data['parent_comment_id']) && $data['parent_comment_id'] !== null && $data['parent_comment_id'] !== '') { + $parentComment = $commentModel->getCommentById((int)$data['parent_comment_id']); + if (!$parentComment || (string)$parentComment['ticket_id'] !== (string)$ticketId) { + http_response_code(400); + ob_end_clean(); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Invalid parent comment']); + exit; + } + } + // Extract @mentions from comment text $mentions = $commentModel->extractMentions($data['comment_text'] ?? ''); $mentionedUsers = []; @@ -130,6 +164,7 @@ try { $authorDisplay = $currentUser['display_name'] ?? $currentUser['username'] ?? null; $commentText = $data['comment_text'] ?? ''; $ticketTitle = $ticket['title'] ?? "Ticket #{$ticketId}"; + $ticketVisibility = $ticket['visibility'] ?? 'public'; // @mention notifications — resolve usernames → Matrix IDs via Synapse Admin API if (!empty($mentionedUsers)) { @@ -142,7 +177,14 @@ try { // General comment notification (opt-in via MATRIX_NOTIFY_COMMENTS) if (!empty($GLOBALS['config']['MATRIX_NOTIFY_COMMENTS'])) { - NotificationHelper::sendCommentNotification($ticketId, $ticketTitle, $commentText, $authorDisplay); + NotificationHelper::sendCommentNotification( + $ticketId, + $ticketTitle, + $commentText, + $authorDisplay, + $ticketVisibility !== 'public', + $ticketVisibility + ); } // Notify watchers of the new comment @@ -152,7 +194,8 @@ try { $ticketTitle, 'comment_added', ['author' => $authorDisplay, 'preview' => mb_strimwidth($commentText, 0, 200, '…')], - (int)$userId + (int)$userId, + $ticketVisibility ); // Add mentioned users to result for frontend diff --git a/api/bootstrap.php b/api/bootstrap.php index 6256ffa..dd2e70c 100644 --- a/api/bootstrap.php +++ b/api/bootstrap.php @@ -34,9 +34,16 @@ if (in_array($_SERVER['REQUEST_METHOD'], ['POST', 'PUT', 'DELETE'])) { require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php'; $csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? ''; if (!CsrfMiddleware::validateToken($csrfToken)) { + // Do NOT rotate on a rejected request. Return the current valid token so a + // client whose token drifted out of sync can recover on its next request + // (the response body is same-origin only, so this can't aid a CSRF attacker). http_response_code(403); header('Content-Type: application/json'); - echo json_encode(['success' => false, 'error' => 'Invalid CSRF token']); + echo json_encode([ + 'success' => false, + 'error' => 'Invalid CSRF token', + 'csrf_token' => CsrfMiddleware::getToken() + ]); exit; } // Rotate token after successful validation; endpoints include it in their JSON response diff --git a/api/bulk_operation.php b/api/bulk_operation.php index 6d0d93a..f25f73c 100644 --- a/api/bulk_operation.php +++ b/api/bulk_operation.php @@ -19,9 +19,9 @@ if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { exit; } -// CSRF Protection +// CSRF Protection for all state-changing methods (any non-GET/HEAD request) require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php'; -if ($_SERVER['REQUEST_METHOD'] === 'POST') { +if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) { $csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? ''; if (!CsrfMiddleware::validateToken($csrfToken)) { http_response_code(403); @@ -47,6 +47,7 @@ $parameters = $data['parameters'] ?? null; // Validate input $validOperationTypes = ['bulk_close', 'bulk_assign', 'bulk_priority', 'bulk_status', 'bulk_delete']; if (!$operationType || !in_array($operationType, $validOperationTypes, true) || empty($ticketIds)) { + http_response_code(400); echo json_encode(['success' => false, 'error' => 'Operation type and ticket IDs required']); exit; } @@ -57,6 +58,7 @@ $ticketIds = array_values(array_filter(array_map(function ($id) { return (ctype_digit($s) && (int)$s > 0) ? $s : null; }, $ticketIds))); if (empty($ticketIds)) { + http_response_code(400); echo json_encode(['success' => false, 'error' => 'No valid ticket IDs provided']); exit; } diff --git a/api/delete_comment.php b/api/delete_comment.php index 85273cf..9b11935 100644 --- a/api/delete_comment.php +++ b/api/delete_comment.php @@ -36,7 +36,11 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { - throw new Exception("Authentication required"); + ob_end_clean(); + http_response_code(401); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Authentication required']); + exit; } // CSRF Protection @@ -64,7 +68,11 @@ try { if (isset($_POST['comment_id'])) { $data = ['comment_id' => $_POST['comment_id']]; } else { - throw new Exception("Missing required field: comment_id"); + ob_end_clean(); + http_response_code(400); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Missing required field: comment_id']); + exit; } } diff --git a/api/ticket_dependencies.php b/api/ticket_dependencies.php index 945cd3d..3c58e87 100644 --- a/api/ticket_dependencies.php +++ b/api/ticket_dependencies.php @@ -80,6 +80,9 @@ if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { $userId = $_SESSION['user']['user_id']; $currentUser = $_SESSION['user']; +$isAdmin = $currentUser['is_admin'] ?? false; +// users.groups is a comma-separated string; the dependency model expects an array. +$userGroups = array_values(array_filter(array_map('trim', explode(',', $currentUser['groups'] ?? '')))); // CSRF Protection for POST/DELETE if ($_SERVER['REQUEST_METHOD'] === 'POST' || $_SERVER['REQUEST_METHOD'] === 'DELETE') { @@ -121,14 +124,14 @@ try { } // Verify user can access this ticket - $ticket = $ticketModel->getTicketById((int)$ticketId); + $ticket = $ticketModel->getTicketById($ticketId); if (!$ticket || !$ticketModel->canUserAccessTicket($ticket, $currentUser)) { ResponseHelper::notFound('Ticket not found'); } try { - $dependencies = $dependencyModel->getDependencies($ticketId); - $dependents = $dependencyModel->getDependentTickets($ticketId); + $dependencies = $dependencyModel->getDependencies($ticketId, $userId, $userGroups, $isAdmin); + $dependents = $dependencyModel->getDependentTickets($ticketId, $userId, $userGroups, $isAdmin); } catch (Exception $e) { error_log('Query error in ticket_dependencies.php GET: ' . $e->getMessage()); ResponseHelper::serverError('Failed to retrieve dependencies'); @@ -157,11 +160,11 @@ try { } // Verify user can access both tickets before creating dependency - $srcTicket = $ticketModel->getTicketById((int)$ticketId); + $srcTicket = $ticketModel->getTicketById($ticketId); if (!$srcTicket || !$ticketModel->canUserAccessTicket($srcTicket, $currentUser)) { ResponseHelper::notFound('Ticket not found'); } - $tgtTicket = $ticketModel->getTicketById((int)$dependsOnId); + $tgtTicket = $ticketModel->getTicketById($dependsOnId); if (!$tgtTicket || !$ticketModel->canUserAccessTicket($tgtTicket, $currentUser)) { ResponseHelper::notFound('Target ticket not found'); } @@ -205,7 +208,7 @@ try { } // Verify user can access the source ticket - $srcTicket = $ticketModel->getTicketById((int)$ticketId); + $srcTicket = $ticketModel->getTicketById($ticketId); if (!$srcTicket || !$ticketModel->canUserAccessTicket($srcTicket, $currentUser)) { ResponseHelper::notFound('Ticket not found'); } @@ -235,7 +238,7 @@ try { ResponseHelper::notFound('Dependency not found'); } - $depTicket = $ticketModel->getTicketById((int)$depRow['ticket_id']); + $depTicket = $ticketModel->getTicketById($depRow['ticket_id']); if (!$depTicket || !$ticketModel->canUserAccessTicket($depTicket, $currentUser)) { ResponseHelper::forbidden('Access denied'); } diff --git a/api/update_comment.php b/api/update_comment.php index 6dc1081..0961053 100644 --- a/api/update_comment.php +++ b/api/update_comment.php @@ -27,12 +27,16 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { - throw new Exception("Authentication required"); + ob_end_clean(); + http_response_code(401); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Authentication required']); + exit; } - // CSRF Protection + // CSRF Protection for all state-changing methods (any non-GET/HEAD request) require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php'; - if ($_SERVER['REQUEST_METHOD'] === 'POST' || $_SERVER['REQUEST_METHOD'] === 'PUT') { + if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) { $csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? ''; if (!CsrfMiddleware::validateToken($csrfToken)) { http_response_code(403); @@ -53,7 +57,11 @@ try { $data = json_decode(file_get_contents('php://input'), true); if (!$data || !isset($data['comment_id']) || !isset($data['comment_text'])) { - throw new Exception("Missing required fields: comment_id, comment_text"); + ob_end_clean(); + http_response_code(400); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Missing required fields: comment_id, comment_text']); + exit; } $commentId = (int)$data['comment_id']; @@ -61,7 +69,11 @@ try { $markdownEnabled = isset($data['markdown_enabled']) && $data['markdown_enabled']; if (empty($commentText)) { - throw new Exception("Comment text cannot be empty"); + ob_end_clean(); + http_response_code(400); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Comment text cannot be empty']); + exit; } // Initialize models diff --git a/api/update_ticket.php b/api/update_ticket.php index 6514ea1..c172ec3 100644 --- a/api/update_ticket.php +++ b/api/update_ticket.php @@ -34,7 +34,11 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { - throw new Exception("Authentication required"); + ob_end_clean(); + http_response_code(401); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Authentication required']); + exit; } // CSRF Protection @@ -115,7 +119,8 @@ try { if (empty($updateData['title'])) { return [ 'success' => false, - 'error' => 'Title cannot be empty' + 'error' => 'Title cannot be empty', + 'http_status' => 400 ]; } @@ -123,7 +128,8 @@ try { if ($updateData['priority'] < 1 || $updateData['priority'] > 5) { return [ 'success' => false, - 'error' => 'Priority must be between 1 and 5' + 'error' => 'Priority must be between 1 and 5', + 'http_status' => 400 ]; } @@ -137,11 +143,32 @@ try { $visibilityGroups = implode(',', array_map('trim', $visibilityGroups)); } + // Authorization: only an admin or the ticket's creator may change + // visibility. Enforce only when the requested visibility actually + // differs so ordinary edits that re-send the same value aren't blocked. + $currentVisibility = $currentTicket['visibility'] ?? 'public'; + $currentGroups = $currentTicket['visibility_groups'] ?? null; + $groupsProvided = array_key_exists('visibility_groups', $data); + $visibilityChanged = ($data['visibility'] !== $currentVisibility) + || ($groupsProvided && (string)$visibilityGroups !== (string)$currentGroups); + if ($visibilityChanged) { + $isCreator = $this->userId !== null + && (int)($currentTicket['created_by'] ?? 0) === (int)$this->userId; + if (!$this->isAdmin && !$isCreator) { + return [ + 'success' => false, + 'error' => 'You do not have permission to change ticket visibility', + 'http_status' => 403 + ]; + } + } + // Internal visibility requires at least one group if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) { return [ 'success' => false, - 'error' => 'Internal visibility requires at least one group to be specified' + 'error' => 'Internal visibility requires at least one group to be specified', + 'http_status' => 400 ]; } } @@ -160,6 +187,19 @@ try { 'error' => 'Status transition not allowed: ' . $currentTicket['status'] . ' → ' . $updateData['status'] ]; } + + // Enforce requires_comment transitions server-side. + if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) { + $comment = trim((string)($data['comment'] ?? $data['comment_text'] ?? '')); + if ($comment === '') { + return [ + 'success' => false, + 'error' => 'A comment is required for this status change', + 'requires_comment' => true, + 'http_status' => 400 + ]; + } + } } // Update ticket with user tracking and optional optimistic locking @@ -257,11 +297,19 @@ try { $data = json_decode($input, true); if (!$data) { - throw new Exception("Invalid JSON data received: " . $input); + ob_end_clean(); + http_response_code(400); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Invalid JSON data received']); + exit; } if (!isset($data['ticket_id'])) { - throw new Exception("Missing ticket_id parameter"); + ob_end_clean(); + http_response_code(400); + header('Content-Type: application/json'); + echo json_encode(['success' => false, 'error' => 'Missing ticket_id parameter']); + exit; } $ticketId = trim((string)$data['ticket_id']); From d11cb989bf17155cb0d0ae47991d77da375d81e1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 12:26:39 -0400 Subject: [PATCH 4/8] Fix API correctness: external API stub/collision, recurring dates, CSV, audit - create_ticket_api.php: remove the wrong CREATE TABLE stub that broke a fresh DB; generate collision-safe ticket_ids so a genuine id collision isn't misreported as a duplicate and a hw alert dropped; stop leaking raw DB errors; correct a reopen comment that falsely claimed refreshed sensor data - manage_recurring.php: fix next-run so create/edit no longer skips the current period (monthly day-of-month this month, daily today if time not passed, correct ISO weekday, month-length clamp); only recompute on schedule changes to avoid double-fire - export_tickets.php, audit_log.php: neutralize CSV formula injection - revoke_api_key.php, generate_api_key.php: correct HTTP status codes and stop the catch clobbering specific 4xx codes - health.php: stop leaking PHP version / extension names / paths to unauthenticated callers - watch_ticket.php: define $data before use - manage_templates/recurring/custom_fields: add audit logging for CRUD; add recurring_ticket + custom_field to the audit entity whitelist Co-Authored-By: Claude Opus 4.8 --- api/audit_log.php | 20 +++++- api/custom_fields.php | 25 +++++++ api/export_tickets.php | 18 ++++- api/generate_api_key.php | 33 +++++++-- api/health.php | 10 ++- api/manage_recurring.php | 149 ++++++++++++++++++++++++++++++++------- api/manage_templates.php | 36 +++++++++- api/revoke_api_key.php | 34 +++++++-- api/watch_ticket.php | 5 +- create_ticket_api.php | 58 +++++++++------ models/AuditLogModel.php | 2 +- 11 files changed, 322 insertions(+), 68 deletions(-) diff --git a/api/audit_log.php b/api/audit_log.php index dc0248d..59a71dd 100644 --- a/api/audit_log.php +++ b/api/audit_log.php @@ -9,6 +9,22 @@ require_once __DIR__ . '/bootstrap.php'; require_once dirname(__DIR__) . '/models/AuditLogModel.php'; +/** + * Neutralize CSV/formula injection: prefix a leading apostrophe to any cell that + * a spreadsheet (Excel/Sheets) would otherwise evaluate as a formula. + * + * @param mixed $value + * @return string + */ +function auditCsvSafeCell($value): string +{ + $value = (string)$value; + if ($value !== '' && in_array($value[0], ['=', '+', '-', '@', "\t", "\r"], true)) { + return "'" . $value; + } + return $value; +} + // Check admin status - audit log viewing is admin-only if (!$isAdmin) { http_response_code(403); @@ -69,7 +85,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { $details = json_encode($log['details']); } - fputcsv($output, [ + fputcsv($output, array_map('auditCsvSafeCell', [ $log['audit_id'] ?? ($log['log_id'] ?? ''), $log['created_at'], $log['display_name'] ?? $log['username'] ?? 'N/A', @@ -78,7 +94,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { $log['entity_id'] ?? 'N/A', $log['ip_address'] ?? 'N/A', $details - ]); + ])); } fclose($output); diff --git a/api/custom_fields.php b/api/custom_fields.php index 50ca414..2ca16a9 100644 --- a/api/custom_fields.php +++ b/api/custom_fields.php @@ -15,6 +15,7 @@ try { require_once dirname(__DIR__) . '/config/config.php'; require_once dirname(__DIR__) . '/helpers/Database.php'; require_once dirname(__DIR__) . '/models/CustomFieldModel.php'; + require_once dirname(__DIR__) . '/models/AuditLogModel.php'; // Check authentication if (session_status() === PHP_SESSION_NONE) { @@ -50,6 +51,8 @@ try { header('Content-Type: application/json'); $model = new CustomFieldModel($conn); + $auditLog = new AuditLogModel($conn); + $currentUserId = $_SESSION['user']['user_id']; $method = $_SERVER['REQUEST_METHOD']; $id = isset($_GET['id']) ? (int)$_GET['id'] : null; $category = isset($_GET['category']) ? $_GET['category'] : null; @@ -75,6 +78,13 @@ try { exit; } $result = $model->createDefinition($data); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'create', 'custom_field', (string)($result['field_id'] ?? ''), [ + 'field_name' => $data['field_name'] ?? null, + 'field_label' => $data['field_label'] ?? null, + 'field_type' => $data['field_type'] ?? null + ]); + } echo json_encode($result); break; @@ -92,6 +102,14 @@ try { exit; } $result = $model->updateDefinition($id, $data); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'update', 'custom_field', (string)$id, [ + 'entity' => 'custom_field', + 'field_name' => $data['field_name'] ?? null, + 'field_label' => $data['field_label'] ?? null, + 'field_type' => $data['field_type'] ?? null + ]); + } echo json_encode($result); break; @@ -102,7 +120,14 @@ try { exit; } + $toDelete = $model->getDefinition($id); $result = $model->deleteDefinition($id); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'delete', 'custom_field', (string)$id, [ + 'entity' => 'custom_field', + 'field_name' => $toDelete['field_name'] ?? 'unknown' + ]); + } echo json_encode($result); break; diff --git a/api/export_tickets.php b/api/export_tickets.php index 549dd42..5972694 100644 --- a/api/export_tickets.php +++ b/api/export_tickets.php @@ -15,6 +15,22 @@ error_reporting(E_ALL); require_once dirname(__DIR__) . '/middleware/RateLimitMiddleware.php'; RateLimitMiddleware::apply('api'); +/** + * Neutralize CSV/formula injection: prefix a leading apostrophe to any cell that + * a spreadsheet (Excel/Sheets) would otherwise evaluate as a formula. + * + * @param mixed $value + * @return string + */ +function exportCsvSafeCell($value): string +{ + $value = (string)$value; + if ($value !== '' && in_array($value[0], ['=', '+', '-', '@', "\t", "\r"], true)) { + return "'" . $value; + } + return $value; +} + try { // Include required files require_once dirname(__DIR__) . '/config/config.php'; @@ -124,7 +140,7 @@ try { $ticket['updated_at'], $ticket['description'] ]; - fputcsv($output, $row); + fputcsv($output, array_map('exportCsvSafeCell', $row)); } fclose($output); diff --git a/api/generate_api_key.php b/api/generate_api_key.php index faca757..77f6dec 100644 --- a/api/generate_api_key.php +++ b/api/generate_api_key.php @@ -24,11 +24,13 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { + http_response_code(401); throw new Exception("Authentication required"); } // Check admin privileges if (!isset($_SESSION['user']['is_admin']) || !$_SESSION['user']['is_admin']) { + http_response_code(403); throw new Exception("Admin privileges required"); } @@ -51,6 +53,7 @@ try { // Get request data $input = json_decode(file_get_contents('php://input'), true); if (!$input) { + http_response_code(400); throw new Exception("Invalid request data"); } @@ -58,10 +61,12 @@ try { $expiresInDays = $input['expires_in_days'] ?? null; if (empty($keyName)) { + http_response_code(400); throw new Exception("Key name is required"); } if (strlen($keyName) > 100) { + http_response_code(400); throw new Exception("Key name must be 100 characters or less"); } @@ -69,6 +74,7 @@ try { if ($expiresInDays !== null && $expiresInDays !== '') { $expiresInDays = (int)$expiresInDays; if ($expiresInDays < 1 || $expiresInDays > 3650) { + http_response_code(400); throw new Exception("Expiration must be between 1 and 3650 days"); } } else { @@ -110,11 +116,26 @@ try { ]); } catch (Exception $e) { ob_end_clean(); - error_log("Generate API key error: " . $e->getMessage()); header('Content-Type: application/json'); - http_response_code(isset($conn) ? 400 : 500); - echo json_encode([ - 'success' => false, - 'error' => 'An internal error occurred' - ]); + + // Preserve any specific status set before the throw (401/403/400/...); + // only fall back to 500 when nothing more specific was set. + $code = http_response_code(); + if (!is_int($code) || $code < 400) { + $code = 500; + } + http_response_code($code); + + if ($code >= 500) { + error_log("Generate API key error: " . $e->getMessage()); + echo json_encode([ + 'success' => false, + 'error' => 'An internal error occurred' + ]); + } else { + echo json_encode([ + 'success' => false, + 'error' => $e->getMessage() + ]); + } } diff --git a/api/health.php b/api/health.php index 6f712f5..908eb31 100644 --- a/api/health.php +++ b/api/health.php @@ -135,11 +135,19 @@ $responseTime = round((microtime(true) - $startTime) * 1000, 2); // Set status code http_response_code($healthy ? 200 : 503); +// This endpoint is unauthenticated, so expose only a coarse per-component status +// and never the diagnostic messages (they leak PHP_VERSION, exact missing +// extension names, and filesystem paths to anonymous callers). +$publicChecks = []; +foreach ($checks as $name => $check) { + $publicChecks[$name] = ['status' => $check['status']]; +} + // Return response echo json_encode([ 'status' => $healthy ? 'healthy' : 'unhealthy', 'timestamp' => date('c'), 'response_time_ms' => $responseTime, - 'checks' => $checks, + 'checks' => $publicChecks, 'version' => '1.0.0' ], JSON_PRETTY_PRINT); diff --git a/api/manage_recurring.php b/api/manage_recurring.php index 96c91c9..79c196e 100644 --- a/api/manage_recurring.php +++ b/api/manage_recurring.php @@ -15,6 +15,7 @@ try { require_once dirname(__DIR__) . '/config/config.php'; require_once dirname(__DIR__) . '/helpers/Database.php'; require_once dirname(__DIR__) . '/models/RecurringTicketModel.php'; + require_once dirname(__DIR__) . '/models/AuditLogModel.php'; // Check authentication if (session_status() === PHP_SESSION_NONE) { @@ -52,6 +53,7 @@ try { header('Content-Type: application/json'); $model = new RecurringTicketModel($conn); + $auditLog = new AuditLogModel($conn); $method = $_SERVER['REQUEST_METHOD']; $id = isset($_GET['id']) ? (int)$_GET['id'] : null; $action = isset($_GET['action']) ? $_GET['action'] : null; @@ -70,6 +72,12 @@ try { case 'POST': if ($action === 'toggle' && $id) { $result = $model->toggleActive($id); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'update', 'recurring_ticket', (string)$id, [ + 'entity' => 'recurring_ticket', + 'action' => 'toggle_active' + ]); + } echo json_encode($result); } else { $data = json_decode(file_get_contents('php://input'), true); @@ -90,6 +98,14 @@ try { $data['created_by'] = $currentUserId; $result = $model->create($data); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'create', 'recurring_ticket', (string)($result['recurring_id'] ?? ''), [ + 'title_template' => $data['title_template'], + 'schedule_type' => $data['schedule_type'], + 'schedule_day' => $data['schedule_day'] ?? null, + 'schedule_time' => $data['schedule_time'] ?? '09:00' + ]); + } echo json_encode($result); } break; @@ -106,16 +122,49 @@ try { exit; } - // Recalculate next run time if schedule changed - $nextRun = calculateNextRun( - $data['schedule_type'], - $data['schedule_day'] ?? null, - $data['schedule_time'] ?? '09:00' - ); - $data['next_run_at'] = $nextRun; + $existing = $model->getById($id); + if (!$existing) { + echo json_encode(['success' => false, 'error' => 'Recurring ticket not found']); + exit; + } + + $newDay = $data['schedule_day'] ?? null; + $newTime = $data['schedule_time'] ?? '09:00'; + + // Only the schedule fields affect when the next occurrence fires. + $scheduleChanged = + (string)$existing['schedule_type'] !== (string)$data['schedule_type'] + || (string)($existing['schedule_day'] ?? '') !== (string)($newDay ?? '') + || substr((string)$existing['schedule_time'], 0, 5) !== substr((string)$newTime, 0, 5); + + $existingNextFuture = !empty($existing['next_run_at']) + && strtotime($existing['next_run_at']) > time(); + + // Recompute only when the schedule actually changed (or the stored + // next_run is already in the past). Editing an unrelated field (e.g. + // title) must NOT move next_run_at backwards past an occurrence that + // may already have fired, which would double-create a ticket. + if ($scheduleChanged || !$existingNextFuture) { + $data['next_run_at'] = calculateNextRun( + $data['schedule_type'], + $newDay, + $newTime + ); + } else { + $data['next_run_at'] = $existing['next_run_at']; + } $data['is_active'] = isset($data['is_active']) ? (int)$data['is_active'] : 1; $result = $model->update($id, $data); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'update', 'recurring_ticket', (string)$id, [ + 'entity' => 'recurring_ticket', + 'title_template' => $data['title_template'] ?? null, + 'schedule_type' => $data['schedule_type'], + 'schedule_day' => $newDay, + 'schedule_time' => $newTime + ]); + } echo json_encode($result); break; @@ -125,7 +174,14 @@ try { exit; } + $toDelete = $model->getById($id); $result = $model->delete($id); + if (!empty($result['success'])) { + $auditLog->log($currentUserId, 'delete', 'recurring_ticket', (string)$id, [ + 'entity' => 'recurring_ticket', + 'title_template' => $toDelete['title_template'] ?? 'unknown' + ]); + } echo json_encode($result); break; @@ -139,36 +195,77 @@ try { echo json_encode(['success' => false, 'error' => 'An internal error occurred']); } -function calculateNextRun($scheduleType, $scheduleDay, $scheduleTime) +/** + * Compute the SOONEST FUTURE occurrence matching the schedule. + * + * Returns 'Y-m-d H:i:s' in the app-configured timezone. The current period is + * NOT skipped: a schedule whose time today/this-month is still in the future + * fires then, not one period later. + * + * @param string $scheduleType daily|weekly|monthly + * @param int|null $scheduleDay 1-7 (ISO, 1=Mon..7=Sun) weekly; 1-31 monthly + * @param string $scheduleTime HH:MM or HH:MM:SS + * @param DateTime|null $now Injected "now" for testing + */ +function calculateNextRun($scheduleType, $scheduleDay, $scheduleTime, ?DateTime $now = null) { - $now = new DateTime(); - $time = $scheduleTime ?: '09:00'; + $tz = new DateTimeZone($GLOBALS['config']['TIMEZONE'] ?? date_default_timezone_get()); + $now = $now ? $now : new DateTime('now', $tz); + + $parts = explode(':', $scheduleTime ?: '09:00'); + $hour = (int)($parts[0] ?? 9); + $minute = (int)($parts[1] ?? 0); + $second = (int)($parts[2] ?? 0); + + $next = clone $now; switch ($scheduleType) { - case 'daily': - $next = new DateTime('tomorrow ' . $time); - break; - case 'weekly': - $days = [1 => 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday']; - $dayName = $days[(int)$scheduleDay] ?? 'Monday'; - $next = new DateTime("next {$dayName} " . $time); + $targetDow = (int)$scheduleDay; + if ($targetDow < 1 || $targetDow > 7) { + $targetDow = 1; + } + $next->setTime($hour, $minute, $second); + $currentDow = (int)$next->format('N'); // 1=Mon .. 7=Sun + $daysAhead = ($targetDow - $currentDow + 7) % 7; + // Same weekday but the time already passed today -> next week. + if ($daysAhead === 0 && $next <= $now) { + $daysAhead = 7; + } + if ($daysAhead > 0) { + $next->modify("+{$daysAhead} day"); + $next->setTime($hour, $minute, $second); + } break; case 'monthly': $day = max(1, min(31, (int)$scheduleDay)); - $next = new DateTime(); - $next->modify('first day of next month'); - // Clamp to last day of target month (handles Feb, 30-day months) - $daysInMonth = (int)$next->format('t'); - $day = min($day, $daysInMonth); - $next->setDate((int)$next->format('Y'), (int)$next->format('m'), $day); - $parts = explode(':', $time . ':00'); // ensure at least H:M - $next->setTime((int)$parts[0], (int)$parts[1], 0); + // This month first, clamped to the month's length (e.g. day 31 -> Feb 28/29). + $daysInMonth = (int)$now->format('t'); + $next->setDate((int)$now->format('Y'), (int)$now->format('n'), min($day, $daysInMonth)); + $next->setTime($hour, $minute, $second); + if ($next <= $now) { + // Already passed this month -> first day of next month, then clamp. + $firstNext = clone $now; + $firstNext->modify('first day of next month'); + $daysInMonth = (int)$firstNext->format('t'); + $next->setDate( + (int)$firstNext->format('Y'), + (int)$firstNext->format('n'), + min($day, $daysInMonth) + ); + $next->setTime($hour, $minute, $second); + } break; + case 'daily': default: - $next = new DateTime('tomorrow ' . $time); + $next->setTime($hour, $minute, $second); + if ($next <= $now) { + $next->modify('+1 day'); + $next->setTime($hour, $minute, $second); + } + break; } return $next->format('Y-m-d H:i:s'); diff --git a/api/manage_templates.php b/api/manage_templates.php index abd9f00..e89067d 100644 --- a/api/manage_templates.php +++ b/api/manage_templates.php @@ -14,6 +14,7 @@ RateLimitMiddleware::apply('api'); try { require_once dirname(__DIR__) . '/config/config.php'; require_once dirname(__DIR__) . '/helpers/Database.php'; + require_once dirname(__DIR__) . '/models/AuditLogModel.php'; // Check authentication if (session_status() === PHP_SESSION_NONE) { @@ -48,6 +49,8 @@ try { header('Content-Type: application/json'); + $auditLog = new AuditLogModel($conn); + $currentUserId = $_SESSION['user']['user_id']; $method = $_SERVER['REQUEST_METHOD']; $id = isset($_GET['id']) ? (int)$_GET['id'] : null; @@ -110,7 +113,13 @@ try { ); if ($stmt->execute()) { - echo json_encode(['success' => true, 'template_id' => $conn->insert_id]); + $newTemplateId = $conn->insert_id; + $auditLog->log($currentUserId, 'create', 'template', (string)$newTemplateId, [ + 'template_name' => $templateName, + 'category' => $category, + 'type' => $type + ]); + echo json_encode(['success' => true, 'template_id' => $newTemplateId]); } else { error_log("Template creation failed: " . $stmt->error); echo json_encode(['success' => false, 'error' => 'Failed to create template']); @@ -161,7 +170,15 @@ try { $id ); - echo json_encode(['success' => $stmt->execute()]); + $updated = $stmt->execute(); + if ($updated) { + $auditLog->log($currentUserId, 'update', 'template', (string)$id, [ + 'template_name' => $templateName, + 'category' => $category, + 'type' => $type + ]); + } + echo json_encode(['success' => $updated]); $stmt->close(); break; @@ -171,9 +188,22 @@ try { exit; } + // Capture the name before deletion for the audit record. + $nameStmt = $conn->prepare("SELECT template_name FROM ticket_templates WHERE template_id = ?"); + $nameStmt->bind_param('i', $id); + $nameStmt->execute(); + $delRow = $nameStmt->get_result()->fetch_assoc(); + $nameStmt->close(); + $stmt = $conn->prepare("DELETE FROM ticket_templates WHERE template_id = ?"); $stmt->bind_param('i', $id); - echo json_encode(['success' => $stmt->execute()]); + $deleted = $stmt->execute(); + if ($deleted) { + $auditLog->log($currentUserId, 'delete', 'template', (string)$id, [ + 'template_name' => $delRow['template_name'] ?? 'unknown' + ]); + } + echo json_encode(['success' => $deleted]); $stmt->close(); break; diff --git a/api/revoke_api_key.php b/api/revoke_api_key.php index e7f8d40..fe2bb7f 100644 --- a/api/revoke_api_key.php +++ b/api/revoke_api_key.php @@ -24,11 +24,13 @@ try { session_start(); } if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { + http_response_code(401); throw new Exception("Authentication required"); } // Check admin privileges if (!isset($_SESSION['user']['is_admin']) || !$_SESSION['user']['is_admin']) { + http_response_code(403); throw new Exception("Admin privileges required"); } @@ -51,12 +53,14 @@ try { // Get request data $input = json_decode(file_get_contents('php://input'), true); if (!$input) { + http_response_code(400); throw new Exception("Invalid request data"); } $keyId = (int)($input['key_id'] ?? 0); if ($keyId <= 0) { + http_response_code(400); throw new Exception("Valid key ID is required"); } @@ -68,10 +72,12 @@ try { $keyInfo = $apiKeyModel->getKeyById($keyId); if (!$keyInfo) { + http_response_code(404); throw new Exception("API key not found"); } if (!$keyInfo['is_active']) { + http_response_code(409); throw new Exception("API key is already revoked"); } @@ -79,6 +85,7 @@ try { $success = $apiKeyModel->revokeKey($keyId); if (!$success) { + http_response_code(500); throw new Exception("Failed to revoke API key"); } @@ -103,11 +110,26 @@ try { ]); } catch (Exception $e) { ob_end_clean(); - error_log("Revoke API key error: " . $e->getMessage()); header('Content-Type: application/json'); - http_response_code(isset($conn) ? 400 : 500); - echo json_encode([ - 'success' => false, - 'error' => 'An internal error occurred' - ]); + + // Preserve any specific status set before the throw (401/403/404/409/...); + // only fall back to 500 when nothing more specific was set. + $code = http_response_code(); + if (!is_int($code) || $code < 400) { + $code = 500; + } + http_response_code($code); + + if ($code >= 500) { + error_log("Revoke API key error: " . $e->getMessage()); + echo json_encode([ + 'success' => false, + 'error' => 'An internal error occurred' + ]); + } else { + echo json_encode([ + 'success' => false, + 'error' => $e->getMessage() + ]); + } } diff --git a/api/watch_ticket.php b/api/watch_ticket.php index 375dc69..10c7f3e 100644 --- a/api/watch_ticket.php +++ b/api/watch_ticket.php @@ -10,12 +10,13 @@ require_once __DIR__ . '/bootstrap.php'; 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'] - : (isset($data['ticket_id']) ? (int)$data['ticket_id'] : 0); + : (int)($data['ticket_id'] ?? 0); if ($_SERVER['REQUEST_METHOD'] === 'POST') { - $data = json_decode(file_get_contents('php://input'), true) ?? []; $ticketId = (int)($data['ticket_id'] ?? 0); $action = $data['action'] ?? ''; diff --git a/create_ticket_api.php b/create_ticket_api.php index 99eab78..3eeffed 100644 --- a/create_ticket_api.php +++ b/create_ticket_api.php @@ -73,18 +73,6 @@ try { $userId = $systemUser['user_id']; -// Create tickets table with hash column if not exists -$createTableSQL = "CREATE TABLE IF NOT EXISTS tickets ( - id INT AUTO_INCREMENT PRIMARY KEY, - ticket_id VARCHAR(9) NOT NULL, - title VARCHAR(255) NOT NULL, - hash VARCHAR(64) NOT NULL, - created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, - UNIQUE KEY unique_hash (hash) -)"; - -$conn->query($createTableSQL); - // Parse input regardless of content-type header $rawInput = file_get_contents('php://input'); $data = json_decode($rawInput, true); @@ -371,7 +359,8 @@ if ($existing) { $reopenStmt->close(); $commentText = "**Issue recurred — ticket reopened automatically.**\n\n" . - "hwmonDaemon detected this condition again. Current sensor data is in the ticket description above."; + "hwmonDaemon detected this condition again. The ticket description reflects the " + . "original report; see this comment's timestamp for when the issue recurred."; $commentStmt = $conn->prepare( "INSERT INTO ticket_comments (ticket_id, user_id, user_name, comment_text, markdown_enabled) VALUES (?, ?, 'hwmonDaemon', ?, 1)" ); @@ -404,13 +393,40 @@ if ($existing) { exit; } -// No existing ticket — create a new one -// Use random_int range 100000000-999999999 to avoid leading-zero IDs -try { - $ticket_id = (string)random_int(100000000, 999999999); -} catch (Exception $e) { - $ticket_id = (string)mt_rand(100000000, 999999999); +// No existing ticket — create a new one. +// Generate a collision-safe unique ticket_id with a pre-check + retry loop (same +// approach as TicketModel::createTicket) so a ticket_id clash cannot happen. That +// way a 1062 on INSERT below can only be the unique_hash (dedup) key racing, and +// is correctly reported as a duplicate rather than a dropped hardware alert. +$ticket_id = null; +$maxAttempts = 50; +$attempts = 0; +do { + try { + $candidateId = sprintf('%09d', random_int(100000000, 999999999)); + } catch (Exception $e) { + $candidateId = sprintf('%09d', mt_rand(100000000, 999999999)); + } + + $idCheckStmt = $conn->prepare("SELECT ticket_id FROM tickets WHERE ticket_id = ? LIMIT 1"); + $idCheckStmt->bind_param("s", $candidateId); + $idCheckStmt->execute(); + $idExists = $idCheckStmt->get_result()->num_rows > 0; + $idCheckStmt->close(); + + if (!$idExists) { + $ticket_id = $candidateId; + } + $attempts++; +} while ($ticket_id === null && $attempts < $maxAttempts); + +if ($ticket_id === null) { + error_log('create_ticket_api: failed to generate a unique ticket_id after ' . $maxAttempts . ' attempts'); + http_response_code(500); + echo json_encode(['success' => false, 'error' => 'Internal server error']); + exit; } + $insertStmt = $conn->prepare( "INSERT INTO tickets (ticket_id, title, description, status, priority, category, type, hash, created_by) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)" @@ -469,5 +485,7 @@ if ($inserted) { 'message' => 'Ticket created successfully', ]); } else { - echo json_encode(['success' => false, 'error' => $conn->error]); + error_log('create_ticket_api: ticket insert reported failure: ' . $conn->error); + http_response_code(500); + echo json_encode(['success' => false, 'error' => 'Internal server error']); } diff --git a/models/AuditLogModel.php b/models/AuditLogModel.php index 2da878d..f0eea92 100644 --- a/models/AuditLogModel.php +++ b/models/AuditLogModel.php @@ -27,7 +27,7 @@ class AuditLogModel private const VALID_ENTITY_TYPES = [ 'ticket', 'comment', 'user', 'api_key', 'security', 'template', 'attachment', 'ticket_attachments', 'group', - 'dependency', 'workflow_transition' + 'dependency', 'workflow_transition', 'recurring_ticket', 'custom_field' ]; public function __construct($conn) From 113b7f9d3fc6d4b21ba3e3d61b168d1943a4dc47 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 13:50:27 -0400 Subject: [PATCH 5/8] Fix frontend JS: CSRF resync, status-comment flow, markdown/XSS, kanban MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - base.js lt.api: resync window.CSRF_TOKEN from response bodies before throwing on errors and attach err.data/err.status, so a desynced client auto-recovers without a reload - add lt.ticketStatus.submit: status changes that require a comment now prompt, post the comment, and retry update_ticket with it; wired into the ticket dropdown, dashboard quick-status, kanban drag-drop and the 1-4 keyboard shortcuts (bulk ops unchanged) — matches the new server requires_comment enforcement - base.js markdown.render: drop the unsafe marked/markdownit delegation; always use the built-in XSS-safe renderer - ticket.js: XHR upload sends the X-CSRF-Token header and resyncs the token; use lt.escHtml instead of a re-inlined escape chain; @-mention trigger requires a word boundary (no firing inside emails); idempotent, anchor-safe highlightMentions - base.js typeahead: discard out-of-order async results - markdown.js: balanced table tbody/thead; ticket-ref linkification runs after code extraction so #ids inside code aren't linked - dashboard.js kanban: don't swallow the click after a drag - keyboard-shortcuts.js: J/K skip hidden/skeleton rows; drop duplicate ? Co-Authored-By: Claude Opus 4.8 --- assets/js/base.js | 108 ++++++++++++++++++++++++++++++-- assets/js/dashboard.js | 29 +++++---- assets/js/keyboard-shortcuts.js | 24 +++++-- assets/js/markdown.js | 32 ++++++---- assets/js/ticket.js | 63 ++++++++++++------- 5 files changed, 200 insertions(+), 56 deletions(-) diff --git a/assets/js/base.js b/assets/js/base.js index 91a8b00..5a4ce3b 100644 --- a/assets/js/base.js +++ b/assets/js/base.js @@ -468,7 +468,15 @@ try { resp = await fetch(url, opts); } catch (err) { throw new Error('Network error: ' + err.message); } let data; try { data = await resp.json(); } catch (_) { data = { success: resp.ok }; } - if (!resp.ok) throw new Error(data.error || data.message || 'HTTP ' + resp.status); + // Resync CSRF token from any response body that carries a fresh one + // (bootstrap rotates on success and returns the current token on rejection). + if (data && data.csrf_token) global.CSRF_TOKEN = data.csrf_token; + if (!resp.ok) { + const err = new Error(data.error || data.message || 'HTTP ' + resp.status); + err.data = data; + err.status = resp.status; + throw err; + } return data; } @@ -2004,6 +2012,7 @@ let _focusedIdx = -1; let _items = []; let _debTimer = null; + let _searchSeq = 0; function _render(items, query) { _items = items.slice(0, maxResults); @@ -2028,16 +2037,21 @@ } async function _search(query) { + // Sequence guard: only the latest query is allowed to render, so a slow + // earlier async source() cannot overwrite a newer query's results. + const seq = ++_searchSeq; dropdown.innerHTML = '
Searching…
'; dropdown.classList.add('is-open'); inputEl.setAttribute('aria-busy', 'true'); try { const results = typeof source === 'function' ? await source(query) : source.filter(i => i.label.toLowerCase().includes(query.toLowerCase())); + if (seq !== _searchSeq) return; _render(results, query); } catch(e) { + if (seq !== _searchSeq) return; dropdown.innerHTML = '
Error loading results
'; } finally { - inputEl.setAttribute('aria-busy', 'false'); + if (seq === _searchSeq) inputEl.setAttribute('aria-busy', 'false'); } } @@ -2704,7 +2718,15 @@ } let data; try { data = await resp.json(); } catch (_) { data = { success: resp.ok }; } - if (!resp.ok) throw new Error(data.error || data.message || 'HTTP ' + resp.status); + // Resync CSRF token from any response body that carries a fresh one + // (bootstrap rotates on success and returns the current token on rejection). + if (data && data.csrf_token) global.CSRF_TOKEN = data.csrf_token; + if (!resp.ok) { + const err = new Error(data.error || data.message || 'HTTP ' + resp.status); + err.data = data; + err.status = resp.status; + throw err; + } return data; } api.get = url => _apiFetchAuth('GET', url); @@ -2713,6 +2735,79 @@ api.patch = (u, b) => _apiFetchAuth('PATCH', u, b); api.delete = (u, b) => _apiFetchAuth('DELETE', u, b); + /* ================================================================ + TICKET STATUS CHANGE (comment-aware) + lt.ticketStatus.submit(ticketId, newStatus, { comment? }) → Promise + Posts /api/update_ticket.php. If the server rejects with + requires_comment, opens a comment modal, persists the comment via + /api/add_comment.php, then retries the update once WITH the comment. + Rejects with err.cancelled === true if the user cancels the modal. + ================================================================ */ + function _statusCommentModal(newStatus) { + return new Promise(resolve => { + const modalId = 'ltStatusCommentModal' + Date.now(); + const safeStatus = escHtml(newStatus); + document.body.insertAdjacentHTML('beforeend', + ''); + const modalEl = document.getElementById(modalId); + openModal(modalId); + let done = false; + const finish = (value) => { + if (done) return; + done = true; + closeModal(modalId); + setTimeout(() => { if (modalEl && modalEl.parentNode) modalEl.remove(); }, 300); + resolve(value); + }; + modalEl.querySelector('[data-modal-close]').addEventListener('click', () => finish(null)); + document.getElementById(modalId + '_cancel').addEventListener('click', () => finish(null)); + document.getElementById(modalId + '_confirm').addEventListener('click', () => { + const ta = document.getElementById(modalId + '_comment'); + const comment = ta ? ta.value.trim() : ''; + if (!comment) { if (ta) ta.focus(); toast.warning('Please enter a reason for this status change.'); return; } + finish(comment); + }); + setTimeout(() => { const ta = document.getElementById(modalId + '_comment'); if (ta) ta.focus(); }, 100); + }); + } + + const ticketStatus = { + submit(ticketId, newStatus, opts) { + opts = opts || {}; + const id = String(ticketId); + const payload = { ticket_id: id, status: newStatus }; + if (opts.comment) payload.comment = opts.comment; + return api.post('/api/update_ticket.php', payload).catch(err => { + if (!(err && err.data && err.data.requires_comment)) throw err; + return _statusCommentModal(newStatus).then(comment => { + if (!comment) { + const cancelErr = new Error('Status change cancelled'); + cancelErr.cancelled = true; + throw cancelErr; + } + // Persist the comment, then retry the status change with it included. + return api.post('/api/add_comment.php', { ticket_id: id, comment_text: comment }) + .then(() => api.post('/api/update_ticket.php', { ticket_id: id, status: newStatus, comment: comment })); + }); + }); + }, + }; + /* ================================================================ MODULE 54 — MARKDOWN RENDERER lt.markdown.render(mdString) → HTML string (sanitized) @@ -2722,9 +2817,9 @@ ================================================================ */ const markdown = { render(md) { - // Delegate to window.marked if available - if (global.marked) return global.marked.parse(md); - if (global.markdownit) return global.markdownit().render(md); + // Always use the built-in XSS-safe micro-renderer. Do NOT delegate to + // window.marked / window.markdownit: their raw HTML output is not sanitized + // here, so delegating would enable stored XSS if such a lib were ever loaded. // Micro-renderer: covers headings, bold, italic, code, links, lists, blockquote, hr let html = escHtml(md) // Fenced code blocks @@ -2943,6 +3038,7 @@ lightbox, auth, markdown, + ticketStatus, pagination, sidebarSubmenus: { init: initSidebarSubmenus }, }; diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index 03628a2..526ef54 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -1000,18 +1000,20 @@ function performQuickStatusChange(ticketId) { if (!quickStatusEl) return; const newStatus = quickStatusEl.value; - lt.api.post('/api/update_ticket.php', { ticket_id: ticketId, status: newStatus }) + // Close this modal first so the comment modal (if requires_comment) stacks cleanly. + closeQuickStatusModal(); + + lt.ticketStatus.submit(ticketId, newStatus) .then(data => { - closeQuickStatusModal(); - if (data.success) { + if (data && data.success) { lt.toast.success(`Status updated to ${newStatus}`, 3000); showTableSkeleton(5); setTimeout(() => window.location.reload(), 1000); } else { - lt.toast.error('Error: ' + (data.error || 'Unknown error'), 4000); + lt.toast.error('Error: ' + ((data && data.error) || 'Unknown error'), 4000); } }) .catch(error => { - closeQuickStatusModal(); + if (error && error.cancelled) return; lt.toast.error('Error updating status', 4000); }); } @@ -1168,8 +1170,9 @@ function populateKanbanCards() { card.dataset.ticketId = ticketId; card.dataset.status = status; card.addEventListener('click', (e) => { - // Don't navigate if drag just ended (drag adds/removes is-dragging briefly) - if (card.dataset.dragged) { delete card.dataset.dragged; return; } + // Don't navigate if a drag just ended. The flag is cleared on a timer + // (see handleKanbanSort), so a genuine later click is not swallowed. + if (card.dataset.dragged) return; window.location.href = '/ticket/' + encodeURIComponent(ticketId); }); card.onkeydown = (e) => { if (e.key === 'Enter' || e.key === ' ') card.click(); }; @@ -1214,6 +1217,9 @@ function populateKanbanCards() { movedCard.dataset.status = newStatus; movedCard.dataset.dragged = '1'; + // Clear the drag flag shortly after the drop so it suppresses only the + // synthetic click fired on drop, not the user's next genuine click. + setTimeout(function () { delete movedCard.dataset.dragged; }, 400); // Optimistically update column counts const dec = document.querySelector(`.column-count[data-status="${oldStatus}"]`); @@ -1230,8 +1236,9 @@ function populateKanbanCards() { if (inc) inc.textContent = '(' + Math.max(0, (parseInt(inc.textContent.replace(/\D/g, ''), 10) || 1) - 1) + ')'; }; - // POST status update via the shared wrapper (adds CSRF + JSON, throws on non-2xx) - lt.api.post('/api/update_ticket.php', { ticket_id: String(ticketId), status: newStatus }) + // Submit via the shared comment-aware helper. Dropping to Closed (or + // reopening) prompts for a required comment and retries; cancel reverts. + lt.ticketStatus.submit(String(ticketId), newStatus) .then(function (data) { if (data && data.success) { lt.toast.success('Ticket #' + ticketId + ' → ' + newStatus, 2500); @@ -1241,8 +1248,8 @@ function populateKanbanCards() { revert(); } }) - .catch(function () { - lt.toast.error('Status update failed — reverting'); + .catch(function (error) { + if (!(error && error.cancelled)) lt.toast.error('Status update failed — reverting'); revert(); }); } diff --git a/assets/js/keyboard-shortcuts.js b/assets/js/keyboard-shortcuts.js index d09cea6..f7da6bf 100644 --- a/assets/js/keyboard-shortcuts.js +++ b/assets/js/keyboard-shortcuts.js @@ -6,11 +6,27 @@ // Track currently selected row for J/K navigation let currentSelectedRowIndex = -1; +let lastNavRowCount = -1; + +// Only navigate real, visible rows — skip skeleton placeholders and rows hidden +// by filters/column toggles (offsetParent is null when display:none). +function getNavigableRows() { + return Array.from(document.querySelectorAll('tbody tr')).filter(function(row) { + return !row.classList.contains('lt-skeleton-row') && row.offsetParent !== null; + }); +} function navigateTableRow(direction) { - const rows = document.querySelectorAll('tbody tr'); + const rows = getNavigableRows(); if (rows.length === 0) return; + // Reset the index when the row set changes (e.g. filter/reload) so navigation + // never lands on a stale/hidden index. + if (rows.length !== lastNavRowCount) { + currentSelectedRowIndex = -1; + lastNavRowCount = rows.length; + } + rows.forEach(row => row.classList.remove('keyboard-selected')); if (direction === 'next') { @@ -47,10 +63,8 @@ document.addEventListener('DOMContentLoaded', function() { } }); - // ?: Show keyboard shortcuts help — use the static #lt-keys-help modal in the footer - lt.keys.on('?', function() { - if (window.lt) lt.modal.open('lt-keys-help'); - }); + // Note: the '?' help shortcut is registered by lt.keys.initDefaults(); do not + // re-bind it here or the help modal opens twice. // J: Next row lt.keys.on('j', () => navigateTableRow('next')); diff --git a/assets/js/markdown.js b/assets/js/markdown.js index 77c83f7..27bc63a 100644 --- a/assets/js/markdown.js +++ b/assets/js/markdown.js @@ -41,9 +41,6 @@ function parseMarkdown(markdown) { .replace(/"/g, '"') .replace(/'/g, '''); - // Ticket references (#123456789) - convert to clickable links - html = html.replace(/#(\d{9})\b/g, '#$1'); - // Code blocks (```code```) - preserve content and don't process further const codeBlocks = []; html = html.replace(/```([\s\S]*?)```/g, function(match, code) { @@ -58,6 +55,11 @@ function parseMarkdown(markdown) { return '%%INLINECODE' + (inlineCodes.length - 1) + '%%'; }); + // Ticket references (#123456789) - convert to clickable links. + // Runs AFTER code extraction so a literal #123456789 inside inline/fenced code + // (now replaced by a placeholder) is not turned into a link. + html = html.replace(/#(\d{9})\b/g, '#$1'); + // Tables (must be processed before other block elements) html = parseMarkdownTables(html); @@ -287,25 +289,33 @@ function buildTable(rows) { if (rows.length === 0) return ''; let html = ''; + let inThead = false; + let inTbody = false; - rows.forEach((row, index) => { + rows.forEach((row) => { const cells = row.content.split('|').filter(cell => cell.trim() !== ''); - const tag = row.type === 'header' ? 'th' : 'td'; - const wrapper = row.type === 'header' ? 'thead' : (index === 1 ? 'tbody' : ''); + const isHeader = row.type === 'header'; + const tag = isHeader ? 'th' : 'td'; - if (wrapper === 'thead') html += ''; - if (wrapper === 'tbody') html += ''; + if (isHeader && !inThead) { html += ''; inThead = true; } + if (!isHeader && !inTbody) { + if (inThead) { html += ''; inThead = false; } + html += ''; + inTbody = true; + } html += ''; cells.forEach(cell => { html += `<${tag}>${cell.trim()}`; }); html += ''; - - if (row.type === 'header') html += ''; }); - html += '
'; + // Close whichever section is still open so tags are balanced for header-only, + // body-only, and header+body tables alike. + if (inThead) html += ''; + if (inTbody) html += ''; + html += ''; return html; } diff --git a/assets/js/ticket.js b/assets/js/ticket.js index 747ac1a..09155a0 100644 --- a/assets/js/ticket.js +++ b/assets/js/ticket.js @@ -291,14 +291,8 @@ function addComment() { // For markdown, use parseMarkdown (sanitizes HTML) displayText = parseMarkdown(commentText); } else { - // For non-markdown, convert line breaks to
and escape HTML - displayText = commentText - .replace(/&/g, '&') - .replace(//g, '>') - .replace(/"/g, '"') - .replace(/'/g, ''') - .replace(/\n/g, '
'); + // For non-markdown, escape HTML then convert line breaks to
+ displayText = lt.escHtml(commentText).replace(/\n/g, '
'); } // Add new comment to the list @@ -538,11 +532,12 @@ function updateTicketStatus() { return; } cleanup(true); - // Post comment first, then change status + // Post comment first (persists it), then change status with the same + // comment included so the server's requires_comment check passes. const ticketId = getTicketIdFromUrl(); lt.api.post('/api/add_comment.php', { ticket_id: ticketId, comment_text: comment }) - .then(() => performStatusChange(statusSelect, selectedOption, newStatus)) - .catch(() => performStatusChange(statusSelect, selectedOption, newStatus)); + .then(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)) + .catch(() => performStatusChange(statusSelect, selectedOption, newStatus, comment)); }); // Focus textarea on open setTimeout(() => { const ta = document.getElementById(`${modalId}_comment`); if (ta) ta.focus(); }, 100); @@ -552,8 +547,11 @@ function updateTicketStatus() { performStatusChange(statusSelect, selectedOption, newStatus); } -// Extract status change logic into reusable function -function performStatusChange(statusSelect, selectedOption, newStatus) { +// Extract status change logic into reusable function. +// `comment` (optional) is included in the update_ticket payload so requires_comment +// transitions pass server validation. lt.ticketStatus.submit handles the +// comment-aware retry if a comment is required but was not pre-collected. +function performStatusChange(statusSelect, selectedOption, newStatus, comment) { const ticketId = getTicketIdFromUrl(); if (!ticketId) { @@ -561,10 +559,10 @@ function performStatusChange(statusSelect, selectedOption, newStatus) { return; } - // Update status via API - lt.api.post('/api/update_ticket.php', { ticket_id: ticketId, status: newStatus }) + // Update status via the shared comment-aware helper + lt.ticketStatus.submit(ticketId, newStatus, { comment: comment }) .then(data => { - if (data.success) { + if (data && data.success) { // Update the dropdown to show new status as current (preserve TDS v1.2 classes) const newClass = 'lt-status-' + newStatus.toLowerCase().replace(/ /g, '-'); statusSelect.className = 'lt-select lt-select-sm lt-status-select ' + newClass; @@ -582,12 +580,14 @@ function performStatusChange(statusSelect, selectedOption, newStatus) { window.location.reload(); }, 500); } else { - lt.toast.error('Error updating status: ' + (data.error || 'Unknown error')); + lt.toast.error('Error updating status: ' + ((data && data.error) || 'Unknown error')); // Reset to current status statusSelect.selectedIndex = 0; } }) .catch(error => { + // User cancelled the required-comment modal — silently revert the dropdown + if (error && error.cancelled) { statusSelect.selectedIndex = 0; return; } lt.toast.error('Error updating status: ' + error.message); // Reset to current status statusSelect.selectedIndex = 0; @@ -938,6 +938,8 @@ function handleFileUpload(files) { if (xhr.status === 200 || xhr.status === 201) { try { const response = JSON.parse(xhr.responseText); + // Keep the CSRF token in sync if the server rotated it + if (response.csrf_token) window.CSRF_TOKEN = response.csrf_token; if (response.success) { if (uploadedCount === totalFiles) { lt.toast.success(`${totalFiles} file(s) uploaded successfully`, 3000); @@ -968,6 +970,9 @@ function handleFileUpload(files) { }); xhr.open('POST', '/api/upload_attachment.php'); + // Send CSRF via header to match the rest of the app (endpoint accepts both + // the X-CSRF-Token header and the csrf_token form field). + if (window.CSRF_TOKEN) xhr.setRequestHeader('X-CSRF-Token', window.CSRF_TOKEN); xhr.send(formData); }); } @@ -1142,12 +1147,17 @@ function handleMentionInput(e) { const text = textarea.value; const cursorPos = textarea.selectionStart; - // Find @ symbol before cursor + // Find @ symbol before cursor. Only trigger when the @ is at a word boundary + // (start of input or preceded by whitespace) so it does not fire inside email + // addresses like foo@bar. let atPos = -1; for (let i = cursorPos - 1; i >= 0; i--) { const char = text[i]; if (char === '@') { - atPos = i; + const prev = i > 0 ? text[i - 1] : ''; + if (i === 0 || /\s/.test(prev)) { + atPos = i; + } break; } if (char === ' ' || char === '\n') { @@ -1277,20 +1287,27 @@ function selectMention(username) { } /** - * Highlight mentions in comment text + * Highlight mentions in comment text. + * Skips content inside existing anchor tags so URLs/emails that contain '@' + * (e.g. auto-linked links or mailto:) are not corrupted or nested. */ function highlightMentions(text) { - return text.replace(/@([a-zA-Z0-9_-]+)/g, '$1'); + return text.replace(/]*>[\s\S]*?<\/a>|@[a-zA-Z0-9_-]+/gi, function (m) { + if (m.charAt(0) === '<') return m; // leave anchor tags untouched + return '' + m.slice(1) + ''; + }); } // Initialize mention autocomplete when DOM is ready document.addEventListener('DOMContentLoaded', function() { initMentionAutocomplete(); - // Highlight @mentions in plain-text comments (markdown.js handles [data-markdown] elements) + // Highlight @mentions in plain-text comments (markdown.js handles [data-markdown] elements). + // Idempotency guard: only process each element once so re-runs don't nest spans. document.querySelectorAll('.comment-text').forEach(el => { - if (!el.hasAttribute('data-markdown')) { + if (!el.hasAttribute('data-markdown') && !el.dataset.mentionsProcessed) { el.innerHTML = highlightMentions(el.innerHTML); + el.dataset.mentionsProcessed = '1'; } }); From 27a5db8c852c49456ce4040b557921f35a63663b Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 15:15:40 -0400 Subject: [PATCH 6/8] Fix views/controllers/router: command palette, create form, admin views - Consolidate the duplicated command palette to a single overlay + init in the footer; fix New Ticket to route to /ticket/create (was a 404 /create); keep the CSP nonce and all commands - TicketController create(): trim title, require a non-empty description, and honor the posted status (validated against the canonical list) instead of silently discarding it - UserActivityView: 'Active Users' counts only users active in the selected range, not every registered user - layout_footer/DashboardView: local esc() now escapes quotes so values used in HTML attributes can't break out - TicketView: comments tab badge shows the true total, not just page one - layout_header: gate the 'View activity log' link behind the admin flag - index.php: validate /admin/user-activity date params; anchor the legacy /ticket.php route; align the audit action-type whitelist with the dropdown - ApiKeysView: correct the external API sample to /create_ticket_api.php Co-Authored-By: Claude Opus 4.8 --- controllers/TicketController.php | 23 +++++++++-- index.php | 16 +++++--- views/DashboardView.php | 2 +- views/TicketView.php | 4 +- views/admin/ApiKeysView.php | 2 +- views/admin/AuditLogView.php | 9 ++++- views/admin/UserActivityView.php | 14 ++++++- views/layout_footer.php | 20 +++++++++- views/layout_header.php | 65 ++------------------------------ 9 files changed, 76 insertions(+), 79 deletions(-) diff --git a/controllers/TicketController.php b/controllers/TicketController.php index 2e2c0d4..5b3adb0 100644 --- a/controllers/TicketController.php +++ b/controllers/TicketController.php @@ -93,19 +93,27 @@ class TicketController $visibilityGroups = implode(',', array_map('trim', $_POST['visibility_groups'])); } + // Honor the posted status, validated against the app's canonical list + $validStatuses = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed']; + $status = $_POST['status'] ?? 'Open'; + if (!in_array($status, $validStatuses, true)) { + $status = 'Open'; + } + $ticketData = [ - 'title' => $_POST['title'] ?? '', + 'title' => trim($_POST['title'] ?? ''), 'description' => $_POST['description'] ?? '', 'priority' => $_POST['priority'] ?? '4', 'category' => $_POST['category'] ?? 'General', 'type' => $_POST['type'] ?? 'Issue', + 'status' => $status, 'visibility' => $_POST['visibility'] ?? 'public', 'visibility_groups' => $visibilityGroups, 'assigned_to' => !empty($_POST['assigned_to']) ? $_POST['assigned_to'] : null ]; - // Validate input - if (empty($ticketData['title'])) { + // Validate input (server-side; form is novalidate) + if ($ticketData['title'] === '') { $error = "Title is required"; $templates = $this->templateModel->getAllTemplates(); $allUsers = $this->userModel->getAllUsers(); @@ -114,6 +122,15 @@ class TicketController return; } + if (trim($ticketData['description']) === '') { + $error = "Description is required"; + $templates = $this->templateModel->getAllTemplates(); + $allUsers = $this->userModel->getAllUsers(); + $conn = $this->conn; // Make $conn available to view + include dirname(__DIR__) . '/views/CreateTicketView.php'; + return; + } + // Create ticket with user tracking $result = $this->ticketModel->createTicket($ticketData, $userId); diff --git a/index.php b/index.php index 602f671..f9732ed 100644 --- a/index.php +++ b/index.php @@ -249,8 +249,11 @@ switch (true) { $params = []; $types = ''; - $allowedActionTypes = ['create','update','delete','comment','assign','status_change','login','security', - 'ticket_create','ticket_update','ticket_delete','attachment_delete','attachment_upload']; + // Mirrors AuditLogModel::VALID_ACTION_TYPES so every option offered by the + // audit-log filter dropdown is actually accepted here. + $allowedActionTypes = ['create','update','delete','view','security_event', + 'login','logout','assign','unassign','comment','mention', + 'revoke','attachment_upload','attachment_delete','bulk_update']; if (!empty($_GET['action_type']) && in_array($_GET['action_type'], $allowedActionTypes, true)) { $whereConditions[] = "al.action_type = ?"; $params[] = $_GET['action_type']; @@ -335,9 +338,12 @@ switch (true) { case $requestPath == '/admin/user-activity': requireAdmin($currentUser); + // Validate date params (YYYY-MM-DD) like the audit-log route; fall back to defaults on garbage + $uaFrom = $_GET['date_from'] ?? ''; + $uaTo = $_GET['date_to'] ?? ''; $dateRange = [ - 'from' => $_GET['date_from'] ?? date('Y-m-d', strtotime('-30 days')), - 'to' => $_GET['date_to'] ?? date('Y-m-d') + 'from' => preg_match('/^\d{4}-\d{2}-\d{2}$/', $uaFrom) ? $uaFrom : date('Y-m-d', strtotime('-30 days')), + 'to' => preg_match('/^\d{4}-\d{2}-\d{2}$/', $uaTo) ? $uaTo : date('Y-m-d') ]; // Optimized query using LEFT JOINs with aggregated subqueries instead of correlated subqueries @@ -410,7 +416,7 @@ switch (true) { header("Location: /"); exit; - case preg_match('/^\/ticket\.php/', $requestPath) && isset($_GET['id']): + case preg_match('/^\/ticket\.php$/', $requestPath) && isset($_GET['id']): $legacyId = (string)$_GET['id']; if (ctype_digit($legacyId) && (int)$legacyId > 0) { header("Location: /ticket/" . $legacyId); diff --git a/views/DashboardView.php b/views/DashboardView.php index 4384d98..af6895d 100644 --- a/views/DashboardView.php +++ b/views/DashboardView.php @@ -1317,7 +1317,7 @@ if (advForm) advForm.addEventListener('submit', function(e) { var pLabels = { '1':'P1 — Critical', '2':'P2 — High', '3':'P3 — Medium', '4':'P4 — Low', '5':'P5 — Minimal' }; var dotClass = { 'Open':'lt-dot-up', 'In Progress':'lt-dot-warn', 'Pending':'lt-dot--orange', 'Closed':'lt-dot-idle' }; - function esc(s) { return String(s||'').replace(/&/g,'&').replace(//g,'>'); } + function esc(s) { return String(s||'').replace(/&/g,'&').replace(//g,'>').replace(/"/g,'"').replace(/'/g,'''); } function fmtAge(dateStr) { var d = new Date(dateStr); diff --git a/views/TicketView.php b/views/TicketView.php index 635c5f9..767ffc0 100644 --- a/views/TicketView.php +++ b/views/TicketView.php @@ -461,8 +461,8 @@ include __DIR__ . '/layout_header.php';