From 99c840fce065a87a47236021bf8f3538636a8453 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 30 Jun 2026 14:21:35 -0400 Subject: [PATCH] Fix logic bugs found in third multi-agent review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Medium: - create_ticket_api.php: environment tags were parsed with explode('][') which left brackets on the first/last tag so the whitelist never matched, dropping the env tag from the dedup hash — a [production] and [staging] issue with otherwise-identical components could collide onto one ticket. Use a bracket-aware regex. - CommentModel::getThreadedCommentsPaged only fetched DIRECT children of root comments, so when pagination is active, nested replies at depth 2-3 vanished from the thread. Expand replies level-by-level (bounded to depth 3). - StatsModel::getTicketsByAssignee ignored the visibility filter the rest of the stats apply, so a non-admin's "by assignee" widget counted (leaked) confidential tickets. Thread the same filter through. - watch_ticket.php GET path returned watch state / watcher names / count for any ticket with no access check (the POST path checks it) — added canUserAccessTicket. - dashboard.js kanban: every card rendered as P4 because the [class*="lt-p"] selector never matched the lt-badge-p1 class and the fallback didn't strip "P". Extract the digit directly. Low: - audit_log.php CSV: "Log ID" column was always blank ($log['log_id'] vs the real audit_id column). Use audit_id. - check_duplicates.php: the graceful-degradation try/catch only covered the throw path; guard the false-return (non-exception mysqli) path too. - notifications.php: owner-who-is-also-@mentioned got two notifications for one comment; drop the duplicate comment row when a mention covers the same comment. - dashboard.js hover preview rendered "PP1" (doubled prefix); strip the leading P. - markdown.js: code/inline-code restore used string replace, so $&, $$, $`, $' in user code were treated as replacement patterns; use a function replacer. Also removed an unused loop var. Co-Authored-By: Claude Opus 4.8 --- api/audit_log.php | 2 +- api/check_duplicates.php | 5 ++++ api/notifications.php | 17 +++++++++++++ api/watch_ticket.php | 11 ++++++++ assets/js/dashboard.js | 10 +++++--- assets/js/markdown.js | 10 +++++--- create_ticket_api.php | 15 ++++++++--- models/CommentModel.php | 54 +++++++++++++++++++++++++--------------- models/StatsModel.php | 31 +++++++++++++++++------ 9 files changed, 115 insertions(+), 40 deletions(-) diff --git a/api/audit_log.php b/api/audit_log.php index b4fb78c..dc0248d 100644 --- a/api/audit_log.php +++ b/api/audit_log.php @@ -70,7 +70,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'GET') { } fputcsv($output, [ - $log['log_id'], + $log['audit_id'] ?? ($log['log_id'] ?? ''), $log['created_at'], $log['display_name'] ?? $log['username'] ?? 'N/A', $log['action_type'], diff --git a/api/check_duplicates.php b/api/check_duplicates.php index 1fb36d2..1cb85f8 100644 --- a/api/check_duplicates.php +++ b/api/check_duplicates.php @@ -64,6 +64,11 @@ try { } $stmt->execute(); $result = $stmt->get_result(); + if ($result === false) { + // Non-exception mysqli mode: execute/get_result return false instead of + // throwing. Treat as a query failure so we don't fatal on $result below. + throw new RuntimeException('query failed: ' . $conn->error); + } } catch (Throwable $e) { error_log('check_duplicates: ' . $e->getMessage()); ResponseHelper::success(['duplicates' => []]); diff --git a/api/notifications.php b/api/notifications.php index c46a4e7..c3d6449 100644 --- a/api/notifications.php +++ b/api/notifications.php @@ -175,6 +175,23 @@ $stmt->execute(); $mentionRows = $stmt->get_result()->fetch_all(MYSQLI_ASSOC); $stmt->close(); +// If the user owns/watches a ticket AND was @mentioned in the same comment, the +// comment query and the mention query both produce a row for it. Prefer the more +// specific mention and drop the duplicate comment notification for that comment. +$mentionCommentIds = []; +foreach ($mentionRows as $mr) { + $md = json_decode($mr['details'] ?? '{}', true) ?? []; + if (!empty($md['comment_id'])) { + $mentionCommentIds[(int)$md['comment_id']] = true; + } +} +if (!empty($mentionCommentIds)) { + $commentRows = array_filter( + $commentRows, + fn($cr) => !isset($mentionCommentIds[(int)($cr['entity_id'] ?? 0)]) + ); +} + // Merge, deduplicate by log_id, sort by created_at desc $all = []; $seen = []; diff --git a/api/watch_ticket.php b/api/watch_ticket.php index 6889e91..375dc69 100644 --- a/api/watch_ticket.php +++ b/api/watch_ticket.php @@ -78,6 +78,17 @@ if ($ticketId <= 0) { exit; } +// Enforce ticket visibility before returning watch state / watcher names, so a +// restricted ticket's watcher list and count aren't disclosed (the POST path +// already checks this). +$ticketModel = new TicketModel($conn); +$ticket = $ticketModel->getTicketById($ticketId); +if (!$ticket || !$ticketModel->canUserAccessTicket($ticket, $currentUser)) { + http_response_code(404); + echo json_encode(['success' => false, 'error' => 'Ticket not found']); + exit; +} + $watchingStmt = $conn->prepare( "SELECT COUNT(*) as cnt FROM ticket_watchers WHERE ticket_id = ? AND user_id = ?" ); diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index 2c542d7..03628a2 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -1142,8 +1142,10 @@ function populateKanbanCards() { if (cells.length < 6) return; const ticketId = cells[0 + offset]?.querySelector('.ticket-link')?.textContent.trim() || ''; - const priorityEl = cells[1 + offset]?.querySelector('[class*="lt-p"]'); - const priority = priorityEl ? priorityEl.textContent.trim().replace('P','') : cells[1 + offset]?.textContent.trim() || '4'; + // The priority cell renders a "P1".."P5" badge; extract just the digit. + // (The old [class*="lt-p"] selector never matched the lt-badge-p1 class, so + // every card fell back to P4 regardless of real priority.) + const priority = (cells[1 + offset]?.textContent.trim() || '').replace(/[^0-9]/g, '') || '4'; const title = cells[2 + offset]?.textContent.trim() || ''; const category = cells[3 + offset]?.textContent.trim() || ''; const statusEl = cells[5 + offset]?.querySelector('.lt-status'); @@ -1315,7 +1317,9 @@ function showTicketPreview(event) { const offset = isAdmin ? 1 : 0; const ticketId = link.textContent.trim(); - const priority = cells[1 + offset]?.textContent.trim() || ''; + // Cell text is already "P1".."P5"; strip the leading P so the template's + // `P${priority}` doesn't render "PP1". + const priority = (cells[1 + offset]?.textContent.trim() || '').replace(/^P/i, ''); const title = cells[2 + offset]?.textContent.trim() || ''; const category = cells[3 + offset]?.textContent.trim() || ''; const type = cells[4 + offset]?.textContent.trim() || ''; diff --git a/assets/js/markdown.js b/assets/js/markdown.js index dc59958..77c83f7 100644 --- a/assets/js/markdown.js +++ b/assets/js/markdown.js @@ -142,12 +142,14 @@ function parseMarkdown(markdown) { html = html.replace(/ \n/g, '
'); html = html.replace(/\n\n/g, '

'); - // Restore code blocks and inline code + // Restore code blocks and inline code. Use a function replacer so '$' + // sequences in user code (e.g. $&, $$, $`, $') are inserted literally rather + // than interpreted as String.replace replacement patterns. codeBlocks.forEach((block, i) => { - html = html.replace('%%CODEBLOCK' + i + '%%', block); + html = html.replace('%%CODEBLOCK' + i + '%%', () => block); }); inlineCodes.forEach((code, i) => { - html = html.replace('%%INLINECODE' + i + '%%', code); + html = html.replace('%%INLINECODE' + i + '%%', () => code); }); // Restore footnote reference placeholders @@ -164,7 +166,7 @@ function parseMarkdown(markdown) { // Append footnote definitions block if (footnoteOrder.length) { html += '

    '; - footnoteOrder.forEach(function(label, i) { + footnoteOrder.forEach(function(label) { html += '
  1. ' + parseMarkdown(footnotes[label]).replace(/<\/?p>/g, '') + ' ↩
    2. '; diff --git a/create_ticket_api.php b/create_ticket_api.php index 0e9e1e9..f49f939 100644 --- a/create_ticket_api.php +++ b/create_ticket_api.php @@ -195,10 +195,17 @@ function generateTicketHash($data) 'source_type' => $sourceType, 'issue_category' => $issueCategory, 'issue_subtype' => $issueSubtype, - 'environment_tags' => array_values(array_filter( - explode('][', $title), - fn($tag) => in_array($tag, ['production', 'development', 'staging', 'single-node', 'cluster-wide']) - )), + 'environment_tags' => (function () use ($title) { + // Extract each [bracketed] tag, then keep the known environment ones. + // (explode('][') leaves brackets stuck to the first/last tag, so e.g. + // "[production] ..." never matched and the env tag was dropped from the + // dedup hash — letting prod and staging issues collide onto one ticket.) + preg_match_all('/\[([^\]]+)\]/', $title, $m); + return array_values(array_filter( + $m[1], + fn($tag) => in_array($tag, ['production', 'development', 'staging', 'single-node', 'cluster-wide'], true) + )); + })(), ]; // Manual tickets should be unique by title (so different software installs don't collide) diff --git a/models/CommentModel.php b/models/CommentModel.php index 5013c72..50f609a 100644 --- a/models/CommentModel.php +++ b/models/CommentModel.php @@ -176,27 +176,41 @@ class CommentModel return []; } - // All replies for these root comments (up to 3 levels deep) - $placeholders = implode(',', array_fill(0, count($rootIds), '?')); - $replySql = "SELECT tc.*, u.display_name, u.username - FROM ticket_comments tc - LEFT JOIN users u ON tc.user_id = u.user_id - WHERE tc.ticket_id = ? - AND tc.parent_comment_id IN ($placeholders) - AND tc.parent_comment_id IS NOT NULL - ORDER BY tc.created_at ASC"; - $replyStmt = $this->conn->prepare($replySql); - $types = 'i' . str_repeat('i', count($rootIds)); - $replyStmt->bind_param($types, $ticketId, ...$rootIds); - $replyStmt->execute(); - $replyResult = $replyStmt->get_result(); - $replyStmt->close(); + // Load replies level-by-level under this page's roots. A single + // "parent_comment_id IN (rootIds)" only fetches DIRECT children, so + // grandchildren/great-grandchildren (addComment allows up to depth 3) + // would be missing from the map and dropped by buildCommentThread. + // Expand iteratively until no new replies (bounded by max depth 3). + $parentIds = $rootIds; + $depth = 0; + while (!empty($parentIds) && $depth < 3) { + $placeholders = implode(',', array_fill(0, count($parentIds), '?')); + $replySql = "SELECT tc.*, u.display_name, u.username + FROM ticket_comments tc + LEFT JOIN users u ON tc.user_id = u.user_id + WHERE tc.ticket_id = ? + 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)); + $replyStmt->bind_param($types, $ticketId, ...$parentIds); + $replyStmt->execute(); + $replyResult = $replyStmt->get_result(); + $replyStmt->close(); - while ($row = $replyResult->fetch_assoc()) { - $row['display_name_formatted'] = $row['display_name'] ?: ($row['user_name'] ?? 'Unknown User'); - $row['replies'] = []; - $row['thread_depth'] = $row['thread_depth'] ?? 1; - $commentMap[$row['comment_id']] = $row; + $nextParentIds = []; + while ($row = $replyResult->fetch_assoc()) { + if (isset($commentMap[$row['comment_id']])) { + continue; // guard against cycles / duplicates + } + $row['display_name_formatted'] = $row['display_name'] ?: ($row['user_name'] ?? 'Unknown User'); + $row['replies'] = []; + $row['thread_depth'] = $depth + 1; + $commentMap[$row['comment_id']] = $row; + $nextParentIds[] = $row['comment_id']; + } + $parentIds = $nextParentIds; + $depth++; } $rootComments = []; diff --git a/models/StatsModel.php b/models/StatsModel.php index 0e9133c..e1fd602 100644 --- a/models/StatsModel.php +++ b/models/StatsModel.php @@ -28,8 +28,14 @@ class StatsModel /** * Get tickets by assignee (top 5) */ - public function getTicketsByAssignee(int $limit = 8): array + public function getTicketsByAssignee(int $limit = 8, array $visFilter = []): array { + // Apply the same visibility filter as the rest of the stats so a non-admin's + // assignee widget doesn't count (and thereby leak) confidential tickets. + $visSQL = $visFilter['sql'] ?? ''; + $visParams = $visFilter['params'] ?? []; + $visTypes = $visFilter['types'] ?? ''; + $sql = "SELECT u.user_id, u.display_name, @@ -37,12 +43,20 @@ class StatsModel COUNT(t.ticket_id) as open_count FROM tickets t LEFT JOIN users u ON t.assigned_to = u.user_id - WHERE t.status != 'Closed' AND t.assigned_to IS NOT NULL - GROUP BY t.assigned_to - ORDER BY open_count DESC - LIMIT ?"; + WHERE t.status != 'Closed' AND t.assigned_to IS NOT NULL"; + if ($visSQL !== '') { + $sql .= " AND ($visSQL)"; + } + $sql .= " GROUP BY t.assigned_to + ORDER BY open_count DESC + LIMIT ?"; + + $params = $visParams; + $params[] = $limit; + $types = $visTypes . 'i'; + $stmt = $this->conn->prepare($sql); - $stmt->bind_param('i', $limit); + $stmt->bind_param($types, ...$params); $stmt->execute(); $result = $stmt->get_result(); $data = []; @@ -173,8 +187,9 @@ class StatsModel // Sort priority keys ksort($byPriority); - // Query 3: Get assignee stats (requires JOIN, kept separate) - $byAssignee = $this->getTicketsByAssignee(); + // Query 3: Get assignee stats (requires JOIN, kept separate). Pass the same + // visibility filter so confidential tickets aren't counted for non-admins. + $byAssignee = $this->getTicketsByAssignee(8, $visFilter); return [ 'open_tickets' => (int)($counts['open_tickets'] ?? 0),