From 1617dc5442afde54806e690e8d2ee17ce72ce04a Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:41:24 -0400 Subject: [PATCH 01/29] Fix 'Clear All Filters' to clear the real date-range params (#75) clearAllFilters() deleted the nonexistent date_from/date_to query params. Every actual date filter (sidebar, Advanced Search, saved filters, stat-card links) uses created_from/to, updated_from/to, and closed_from/to, so clicking the button silently left any active date range in place. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/dashboard.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index 6d38c2c..65a13e9 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -297,8 +297,12 @@ function clearAllFilters() { params.delete('type'); params.delete('assigned_to'); params.delete('search'); - params.delete('date_from'); - params.delete('date_to'); + params.delete('created_from'); + params.delete('created_to'); + params.delete('updated_from'); + params.delete('updated_to'); + params.delete('closed_from'); + params.delete('closed_to'); params.delete('page'); // Keep sort parameters From 6183bcd4219714a3436b5fe5ea4f74e95ac95ed8 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:41:29 -0400 Subject: [PATCH 02/29] Notification titles: handle non-status ticket edits (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'update' notification formatter unconditionally read details['status']['from']/['to'], so any title/priority/description/ category/type/visibility-only edit fell through to '?' on both sides and produced a broken "changed status on #123: ? → ?" title regardless of what actually changed. Now it branches on the delta shape actually present: the flat {field, from, to} shape used for visibility changes, then each per-field {from, to} delta in priority order, falling back to a generic "updated ticket" message only if none match. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- api/notifications.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/api/notifications.php b/api/notifications.php index c3d6449..e5c0586 100644 --- a/api/notifications.php +++ b/api/notifications.php @@ -225,10 +225,21 @@ foreach ($all as $row) { 'comment' => "{$row['actor_name']} commented on ticket #{$ticketId}", 'mention' => "{$row['actor_name']} mentioned you on ticket #{$ticketId}", 'update' => (function () use ($row, $details, $ticketId) { - // logTicketUpdate stores delta as {"status": {"from": "Open", "to": "In Progress"}} - $from = $details['status']['from'] ?? ($details['old_value'] ?? '?'); - $to = $details['status']['to'] ?? ($details['new_value'] ?? '?'); - return "{$row['actor_name']} changed status on #{$ticketId}: {$from} → {$to}"; + // Visibility changes log a flat {field, from, to} shape (api/update_ticket.php). + if (isset($details['field'], $details['from'], $details['to'])) { + return "{$row['actor_name']} changed {$details['field']} on #{$ticketId}: {$details['from']} → {$details['to']}"; + } + + // Single/bulk field updates log a per-field delta, e.g. + // {"status": {"from": "Open", "to": "In Progress"}}. Only one field + // changed at a time is reported, in priority order below. + foreach (['status', 'priority', 'title', 'category', 'type', 'description'] as $field) { + if (isset($details[$field]['from'], $details[$field]['to'])) { + return "{$row['actor_name']} changed {$field} on #{$ticketId}: {$details[$field]['from']} → {$details[$field]['to']}"; + } + } + + return "{$row['actor_name']} updated ticket #{$ticketId}"; })(), default => "{$row['actor_name']} updated ticket #{$ticketId}", }; From 1ab4d01a3a57c454cddff6925a8cff3d3024b30d Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:41:47 -0400 Subject: [PATCH 03/29] Fix broken Quick Assign dropdown (#102) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quickAssign() wired lt.combobox.init() with an onSelect callback, but combobox only supports the multi-select onChange(selected[]) contract — onSelect is never invoked, so _quickAssignUserId stayed undefined no matter what the user picked and Quick Assign always showed "Please select a user from the list." Switched to lt.typeahead.init(), which does support onSelect, matching the already-working Bulk Assign modal. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/dashboard.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index 65a13e9..a354ec0 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -1139,12 +1139,11 @@ function quickAssign(ticketId) {

Ticket #${lt.escHtml(String(ticketId))}

-
-
- -
- +
+ +
@@ -1170,7 +1169,9 @@ function quickAssign(ticketId) { label: u.display_name || u.username })) ]; - lt.combobox.init(input, items, { + lt.typeahead.init(input, items, { + minChars: 1, + maxResults: 8, onSelect: function(item) { _quickAssignUserId = item.value || null; } }); } From 2bda60364726ef3061eb1915ab5acdd130859109 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:44:10 -0400 Subject: [PATCH 04/29] Chart click-to-filter now merges into the current query string (#29) gotoFilter() built a brand-new URLSearchParams containing only the clicked chart segment's filter keys, discarding every other active filter (search text, date range, saved-filter selection, etc.) on navigation. Now merges the segment's filter into the current location.search, same fix approach already applied to #22. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- views/DashboardView.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/views/DashboardView.php b/views/DashboardView.php index d210aa2..2db7bed 100644 --- a/views/DashboardView.php +++ b/views/DashboardView.php @@ -292,9 +292,10 @@ include __DIR__ . '/layout_header.php'; } function gotoFilter(params) { - var qs = new URLSearchParams(); + var qs = new URLSearchParams(window.location.search); Object.keys(params).forEach(function(k) { if (params[k] !== null && params[k] !== undefined && params[k] !== '') qs.set(k, params[k]); + else qs.delete(k); }); window.location.href = '/?' + qs.toString(); } From f7872b09806a8e425abc1dd3ab194abba71a7e12 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:44:15 -0400 Subject: [PATCH 05/29] Use showConfirmModal() instead of browser confirm() for template overwrite (#53) CreateTicketView.php was the one remaining spot using the native confirm() dialog, violating README Dev Note #21. Split loadTemplate() into a confirm check + applyTemplate(), routed through the project's styled showConfirmModal(), matching every other destructive-action confirmation in the app. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- views/CreateTicketView.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/views/CreateTicketView.php b/views/CreateTicketView.php index c668cce..7714d4c 100644 --- a/views/CreateTicketView.php +++ b/views/CreateTicketView.php @@ -344,12 +344,23 @@ include __DIR__ . '/layout_header.php'; var existingTitle = (document.getElementById('title').value || '').trim(); var existingDesc = (document.getElementById('description').value || '').trim(); if (existingTitle || existingDesc) { - if (!confirm('Applying this template will overwrite your current title and description. Continue?')) { - document.getElementById('templateSelect').value = ''; - return; - } + showConfirmModal( + 'Overwrite content?', + 'Applying this template will overwrite your current title and description. Continue?', + 'warning', + applyTemplate, + function () { document.getElementById('templateSelect').value = ''; } + ); + return; } + applyTemplate(); + } + + function applyTemplate() { + var tplId = document.getElementById('templateSelect').value; + if (!tplId) return; + lt.api.get('/api/get_template.php?template_id=' + encodeURIComponent(tplId)) .then(function (data) { if (!data.success || !data.template) { From 33de91cc86be4f5443df0602854980f79f1aad32 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:44:19 -0400 Subject: [PATCH 06/29] Delete dead RecurringTicketModel::updateAfterRun() (#90) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero callers anywhere in the codebase — superseded by claimForRun(), which the cron script actually uses and which additionally guards against the double-fire race between concurrent cron invocations that this method lacked. Removing it so a future reuse doesn't silently reintroduce that race. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- models/RecurringTicketModel.php | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/models/RecurringTicketModel.php b/models/RecurringTicketModel.php index 500a14f..58a1cf0 100644 --- a/models/RecurringTicketModel.php +++ b/models/RecurringTicketModel.php @@ -189,30 +189,6 @@ class RecurringTicketModel return $claimed; } - /** - * Update last run and calculate next run time - */ - public function updateAfterRun($recurringId) - { - $recurring = $this->getById($recurringId); - if (!$recurring) { - return false; - } - - $nextRun = $this->calculateNextRunTime( - $recurring['schedule_type'], - $recurring['schedule_day'], - $recurring['schedule_time'] - ); - - $sql = "UPDATE recurring_tickets SET last_run_at = NOW(), next_run_at = ? WHERE recurring_id = ?"; - $stmt = $this->conn->prepare($sql); - $stmt->bind_param('si', $nextRun, $recurringId); - $success = $stmt->execute(); - $stmt->close(); - return $success; - } - /** * Calculate the next run time based on schedule */ From e9494dd4b33c8e099886251b36a3f897598f788d Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:54:45 -0400 Subject: [PATCH 07/29] Use server-verified mime_type for attachment thumbnail detection (#60) renderAttachments() decided whether to render an image thumbnail by regex-matching the display filename extension, rather than the finfo-verified mime_type the API already returns. A file whose real type differs from its display name (e.g. a PDF a user named photo.png) rendered a broken instead of falling back to the file-type icon. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/ticket.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/js/ticket.js b/assets/js/ticket.js index 3495475..316ee34 100644 --- a/assets/js/ticket.js +++ b/assets/js/ticket.js @@ -1048,7 +1048,7 @@ function renderAttachments(attachments) { }); const uploadDate = `${lt.time.ago(att.uploaded_at)}`; - const isImage = /\.(png|jpe?g|gif|webp|svg|bmp)$/i.test(att.original_filename); + const isImage = /^image\//i.test(att.mime_type || ''); const imgUrl = `/api/download_attachment.php?id=${att.attachment_id}&inline=1`; const iconHtml = isImage ? ` From 4d0dc2c2ceeaac3007325f1410a18c9e1c2599c1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:54:51 -0400 Subject: [PATCH 08/29] Remove dead sortTable() and write-only ticketViewMode key (#79) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small dead-code cleanups from a dashboard.js state-management audit: - sortTable(table, column) had zero callers — actual table sorting is wired through lt.sortTable.init() via initTableSorting(). - setViewMode() wrote localStorage['ticketViewMode'], but nothing ever read it back; the real view-mode restoration on page load reads lt_activeTab_, written separately by lt.tabs in base.js. Both looked load-bearing but weren't, risking a future edit assuming otherwise. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/dashboard.js | 68 ------------------------------------------ 1 file changed, 68 deletions(-) diff --git a/assets/js/dashboard.js b/assets/js/dashboard.js index a354ec0..9b431c7 100644 --- a/assets/js/dashboard.js +++ b/assets/js/dashboard.js @@ -385,73 +385,6 @@ function initSettingsModal() { } } -function sortTable(table, column) { - const headers = table.querySelectorAll('th'); - headers.forEach(header => { - header.classList.remove('sort-asc', 'sort-desc'); - }); - - const rows = Array.from(table.querySelectorAll('tbody tr')); - const currentDirection = table.dataset.sortColumn == column - ? (table.dataset.sortDirection === 'asc' ? 'desc' : 'asc') - : 'asc'; - - table.dataset.sortColumn = column; - table.dataset.sortDirection = currentDirection; - - rows.sort((a, b) => { - const aValue = a.children[column].textContent.trim(); - const bValue = b.children[column].textContent.trim(); - - // Check if this is a date column — prefer data-ts attribute over text (which may be relative) - const headerText = headers[column].textContent.toLowerCase(); - if (headerText === 'created' || headerText === 'updated') { - const cellA = a.children[column]; - const cellB = b.children[column]; - const dateA = new Date(cellA.dataset.ts || aValue); - const dateB = new Date(cellB.dataset.ts || bValue); - return currentDirection === 'asc' ? dateA - dateB : dateB - dateA; - } - - // Special handling for "Assigned To" column - if (headerText === 'assigned to') { - const aUnassigned = aValue === 'Unassigned'; - const bUnassigned = bValue === 'Unassigned'; - - // Both unassigned - equal - if (aUnassigned && bUnassigned) return 0; - - // Put unassigned at the end regardless of sort direction - if (aUnassigned) return 1; - if (bUnassigned) return -1; - - // Otherwise sort names normally - return currentDirection === 'asc' - ? aValue.localeCompare(bValue) - : bValue.localeCompare(aValue); - } - - // Numeric comparison - const numA = parseFloat(aValue); - const numB = parseFloat(bValue); - - if (!isNaN(numA) && !isNaN(numB)) { - return currentDirection === 'asc' ? numA - numB : numB - numA; - } - - // String comparison - return currentDirection === 'asc' - ? aValue.localeCompare(bValue) - : bValue.localeCompare(aValue); - }); - - const currentHeader = headers[column]; - currentHeader.classList.add(currentDirection === 'asc' ? 'sort-asc' : 'sort-desc'); - - const tbody = table.querySelector('tbody'); - rows.forEach(row => tbody.appendChild(row)); -} - // Old settings modal functions removed - now using settings.js with new settings modal @@ -1220,7 +1153,6 @@ function setViewMode(mode) { if (mode === 'card') { populateKanbanCards(); } - localStorage.setItem('ticketViewMode', mode); } /** From e0c7399998cbfe12313c57496395eb776d96eb05 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 10:54:56 -0400 Subject: [PATCH 09/29] Advanced Search: swap inverted date/priority ranges instead of submitting them (#61) A user could set an end date before a start date, or priority_min > priority_max, and the filter would be silently sent as an unsatisfiable range with zero results and no explanation. Now swaps min/max (and from/to) before building the query string when they're in the wrong order. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/advanced-search.js | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/assets/js/advanced-search.js b/assets/js/advanced-search.js index 7155fc5..09c42f9 100644 --- a/assets/js/advanced-search.js +++ b/assets/js/advanced-search.js @@ -87,11 +87,17 @@ function performAdvancedSearch(event) { params.set('search', searchText); } - // Date ranges - const createdFrom = document.getElementById('adv-created-from').value; - const createdTo = document.getElementById('adv-created-to').value; - const updatedFrom = document.getElementById('adv-updated-from').value; - const updatedTo = document.getElementById('adv-updated-to').value; + // Date ranges — swap if the user entered an end date before the start date + let createdFrom = document.getElementById('adv-created-from').value; + let createdTo = document.getElementById('adv-created-to').value; + if (createdFrom && createdTo && createdFrom > createdTo) { + [createdFrom, createdTo] = [createdTo, createdFrom]; + } + let updatedFrom = document.getElementById('adv-updated-from').value; + let updatedTo = document.getElementById('adv-updated-to').value; + if (updatedFrom && updatedTo && updatedFrom > updatedTo) { + [updatedFrom, updatedTo] = [updatedTo, updatedFrom]; + } if (createdFrom) params.set('created_from', createdFrom); if (createdTo) params.set('created_to', createdTo); @@ -105,9 +111,12 @@ function performAdvancedSearch(event) { params.set('status', selectedStatuses.join(',')); } - // Priority range - const priorityMin = document.getElementById('adv-priority-min').value; - const priorityMax = document.getElementById('adv-priority-max').value; + // Priority range — swap if min > max so the range is always satisfiable + let priorityMin = document.getElementById('adv-priority-min').value; + let priorityMax = document.getElementById('adv-priority-max').value; + if (priorityMin && priorityMax && Number(priorityMin) > Number(priorityMax)) { + [priorityMin, priorityMax] = [priorityMax, priorityMin]; + } if (priorityMin) params.set('priority_min', priorityMin); if (priorityMax) params.set('priority_max', priorityMax); From 6eefeafcbfa5f336627a8abdbc7fa4016c9acb02 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:40:43 -0400 Subject: [PATCH 10/29] Fix avatar color drift between PHP and JS (#31) The JS side claimed to "mirror the PHP crc32 % 4 logic" but actually implemented a different rolling hash (classic String.hashCode()-style), so the same display name could get different avatar colors depending on whether a comment was server-rendered or client-rendered (new comment, reply, watcher avatars, "Load more" pagination). Added a real CRC-32 (IEEE 802.3/zlib polynomial, UTF-8 byte sequence) to ticket.js and switched all three JS call sites (avatarColorClass, watcher avatars, and buildCommentEl in TicketView.php) to use it, verified to produce identical output to PHP's crc32() including for non-ASCII names. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/ticket.js | 32 ++++++++++++++++++++++++++------ views/TicketView.php | 12 +++--------- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/assets/js/ticket.js b/assets/js/ticket.js index 316ee34..a174d9e 100644 --- a/assets/js/ticket.js +++ b/assets/js/ticket.js @@ -183,15 +183,35 @@ function toggleEditMode() { } /** - * Compute avatar color class from display name (mirrors PHP crc32 % 4 logic) + * CRC-32 (IEEE 802.3 / zlib polynomial), matching PHP's crc32(). Operates on + * the UTF-8 byte sequence, same as PHP, so results agree for non-ASCII names. + */ +function crc32(str) { + var bytes = unescape(encodeURIComponent(str)); + var table = crc32._table || (crc32._table = (function () { + var t = []; + for (var n = 0; n < 256; n++) { + var c = n; + for (var k = 0; k < 8; k++) { + c = (c & 1) ? (0xEDB88320 ^ (c >>> 1)) : (c >>> 1); + } + t[n] = c; + } + return t; + })()); + var crc = -1; + for (var i = 0; i < bytes.length; i++) { + crc = (crc >>> 8) ^ table[(crc ^ bytes.charCodeAt(i)) & 0xFF]; + } + return (crc ^ -1) >>> 0; +} + +/** + * Compute avatar color class from display name (mirrors PHP's crc32 % 4 logic) */ function avatarColorClass(displayName) { var colors = ['lt-avatar--orange', 'lt-avatar--green', 'lt-avatar--purple', '']; - var h = 0; - for (var i = 0; i < displayName.length; i++) { - h = ((h << 5) - h + displayName.charCodeAt(i)) | 0; - } - return colors[Math.abs(h) % 4]; + return colors[crc32(displayName) % 4]; } /** diff --git a/views/TicketView.php b/views/TicketView.php index 58ecf95..905cb80 100644 --- a/views/TicketView.php +++ b/views/TicketView.php @@ -1006,9 +1006,7 @@ document.addEventListener('DOMContentLoaded', function () { shown.forEach(function (w) { var words = (w.display_name || '').trim().split(/\s+/).filter(Boolean); var initials = words.slice(0, 2).map(function (x) { return x[0].toUpperCase(); }).join(''); - var hash = 0; - for (var i = 0; i < (w.display_name || '').length; i++) hash = ((hash << 5) - hash + (w.display_name || '').charCodeAt(i)) | 0; - var color = avatarColors[Math.abs(hash) % 4]; + var color = avatarColors[crc32(w.display_name || '') % 4]; html += '
' + '' + '' + lt.escHtml(initials) + '' + @@ -1252,13 +1250,9 @@ document.addEventListener('DOMContentLoaded', function () { var words = displayName.trim().split(/\s+/).filter(Boolean); var initials = words.slice(0, 2).map(function (w) { return w[0].toUpperCase(); }).join(''); - // Avatar color (same modulo logic as PHP: crc32 mod 4) + // Avatar color (real crc32, matching PHP's crc32 % 4 exactly) var avatarColors = ['lt-avatar--orange', 'lt-avatar--green', 'lt-avatar--purple', '']; - var hash = 0; - for (var i = 0; i < displayName.length; i++) { - hash = ((hash << 5) - hash + displayName.charCodeAt(i)) | 0; - } - var avatarColor = avatarColors[Math.abs(hash) % 4]; + var avatarColor = avatarColors[crc32(displayName) % 4]; // Format date var dateStr = c.created_at || ''; From 7c1c1b61ccf5fd1815de8486932734564adfecab Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:41:30 -0400 Subject: [PATCH 11/29] Fix race in first-time login user creation (#96) syncUserFromAuthelia() did a plain check-then-insert with no transaction, so two simultaneous first-visit requests for the same brand-new user (e.g. two tabs opened right after SSO login) could race: the second INSERT hits users.username's UNIQUE KEY, which mysqli throws on (uncaught, PHP 8.1+ default report mode) rather than returning false. Switched to INSERT ... ON DUPLICATE KEY UPDATE followed by a re-fetch by username, so the losing request updates the winner's row instead of throwing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- models/UserModel.php | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/models/UserModel.php b/models/UserModel.php index 0664d94..1524349 100644 --- a/models/UserModel.php +++ b/models/UserModel.php @@ -98,19 +98,30 @@ class UserModel $user['groups'] = $groups; $user['is_admin'] = $isAdmin; } else { - // Create new user + // Create new user. Uses INSERT ... ON DUPLICATE KEY UPDATE (rather than + // a plain INSERT) so two concurrent first-visit requests for the same + // brand-new username can't race: the losing request updates the row the + // winner just created instead of throwing an uncaught duplicate-key + // exception (users.username has a UNIQUE KEY, and mysqli throws on + // constraint violation under PHP 8.1+'s default report mode). $insertStmt = $this->conn->prepare( - "INSERT INTO users (username, display_name, email, `groups`, is_admin, last_login) VALUES (?, ?, ?, ?, ?, NOW())" + "INSERT INTO users (username, display_name, email, `groups`, is_admin, last_login) + VALUES (?, ?, ?, ?, ?, NOW()) + ON DUPLICATE KEY UPDATE + display_name = VALUES(display_name), + email = VALUES(email), + `groups` = VALUES(groups), + is_admin = VALUES(is_admin), + last_login = NOW()" ); $insertStmt->bind_param("ssssi", $username, $displayName, $email, $groups, $isAdmin); $insertStmt->execute(); - - $userId = $this->conn->insert_id; $insertStmt->close(); - // Get the newly created user - $stmt = $this->conn->prepare("SELECT * FROM users WHERE user_id = ?"); - $stmt->bind_param("i", $userId); + // Re-fetch by username — works whether this request won the insert or + // lost the race and only updated the winner's row. + $stmt = $this->conn->prepare("SELECT * FROM users WHERE username = ?"); + $stmt->bind_param("s", $username); $stmt->execute(); $result = $stmt->get_result(); $user = $result->fetch_assoc(); From 5e8af395633092b9f512d955a99ee1484f2b68cf Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:43:53 -0400 Subject: [PATCH 12/29] Fix collation inconsistency on saved_filters/ticket_attachments (#41) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README Dev Note #12 mandates utf8mb4_general_ci for new tables, but these two tables were created with utf8mb4_unicode_ci in the baseline schema — inconsistent with every other table, and a future join or comparison against a general_ci column would need explicit COLLATE casts or hit "Illegal mix of collations" errors. - Fixed 000_baseline.sql so a fresh install matches the convention directly. - Added 002_fix_collation_consistency.sql for existing deployments. MariaDB silently drops the inline CHECK (json_valid(...)) constraint on saved_filters.filter_criteria when that column is MODIFYed (found by actually running this against a local MariaDB instance), so the migration explicitly re-adds it after the collation conversion. Verified against a local MariaDB 10.11: baseline applies cleanly, migration is idempotent (safe to run twice), and the json_valid CHECK is still enforced afterward. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- migrations/000_baseline.sql | 4 +-- migrations/002_fix_collation_consistency.sql | 30 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 migrations/002_fix_collation_consistency.sql diff --git a/migrations/000_baseline.sql b/migrations/000_baseline.sql index dc7ddc8..3eeee7d 100644 --- a/migrations/000_baseline.sql +++ b/migrations/000_baseline.sql @@ -155,7 +155,7 @@ CREATE TABLE IF NOT EXISTS `saved_filters` ( UNIQUE KEY `unique_user_filter_name` (`user_id`,`filter_name`), KEY `idx_user_filters` (`user_id`,`is_default`), CONSTRAINT `saved_filters_ibfk_1` FOREIGN KEY (`user_id`) REFERENCES `users` (`user_id`) ON DELETE CASCADE -) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; -- ============ status_transitions ============ CREATE TABLE IF NOT EXISTS `status_transitions` ( @@ -185,7 +185,7 @@ CREATE TABLE IF NOT EXISTS `ticket_attachments` ( KEY `idx_attachments_ticket` (`ticket_id`), KEY `idx_attachments_uploaded_by` (`uploaded_by`), CONSTRAINT `ticket_attachments_ibfk_1` FOREIGN KEY (`uploaded_by`) REFERENCES `users` (`user_id`) ON DELETE SET NULL -) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; -- ============ ticket_comments ============ CREATE TABLE IF NOT EXISTS `ticket_comments` ( diff --git a/migrations/002_fix_collation_consistency.sql b/migrations/002_fix_collation_consistency.sql new file mode 100644 index 0000000..5f366ec --- /dev/null +++ b/migrations/002_fix_collation_consistency.sql @@ -0,0 +1,30 @@ +-- Fix collation inconsistency on saved_filters and ticket_attachments +-- +-- README.md Developer Notes #12: "Database collation: Use +-- utf8mb4_general_ci (not unicode_ci) for new tables." These two tables +-- were created with utf8mb4_unicode_ci instead, inconsistent with every +-- other table in the schema. Mixed collations don't break anything by +-- themselves, but any future query joining/comparing these columns +-- against general_ci columns needs explicit COLLATE casts or hits +-- "Illegal mix of collations" errors. +-- +-- Safe to re-run. + +ALTER TABLE `saved_filters` + CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci; + +-- saved_filters.filter_criteria is pinned to utf8mb4_bin (for the +-- json_valid() CHECK constraint) — restore that after the table-wide +-- CONVERT TO above, which resets it to general_ci. MariaDB drops the +-- inline CHECK when the column is MODIFYed, so re-add it explicitly. +ALTER TABLE `saved_filters` + MODIFY COLUMN `filter_criteria` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL; + +ALTER TABLE `saved_filters` + DROP CONSTRAINT IF EXISTS `saved_filters_filter_criteria_json`; + +ALTER TABLE `saved_filters` + ADD CONSTRAINT `saved_filters_filter_criteria_json` CHECK (json_valid(`filter_criteria`)); + +ALTER TABLE `ticket_attachments` + CONVERT TO CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci; From c892d9dcc809b23d3af101d4682a36b2283512f9 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:45:11 -0400 Subject: [PATCH 13/29] Recompute next_run_at when re-enabling a paused recurring schedule (#89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toggleActive() flipped is_active without touching next_run_at. If a schedule was disabled while next_run_at was still in the future, then re-enabled after that date had passed, the next cron tick saw next_run_at <= NOW() and fired immediately — surprising for an admin expecting a re-enabled "daily" schedule to wait until its next natural occurrence. Now recomputes next_run_at from the current time when transitioning to active, matching what a fresh schedule creation would produce; disabling is unchanged. Verified against a local MariaDB instance: re-enabling a schedule whose next_run_at was in 2020 recomputed it to tomorrow at the scheduled time; disabling leaves next_run_at untouched. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- models/RecurringTicketModel.php | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/models/RecurringTicketModel.php b/models/RecurringTicketModel.php index 58a1cf0..d6dc988 100644 --- a/models/RecurringTicketModel.php +++ b/models/RecurringTicketModel.php @@ -231,9 +231,33 @@ class RecurringTicketModel */ public function toggleActive($recurringId) { - $sql = "UPDATE recurring_tickets SET is_active = NOT is_active WHERE recurring_id = ?"; - $stmt = $this->conn->prepare($sql); - $stmt->bind_param('i', $recurringId); + $recurring = $this->getById($recurringId); + if (!$recurring) { + return ['success' => false]; + } + + $newActive = $recurring['is_active'] ? 0 : 1; + + if ($newActive) { + // Re-enabling: recompute next_run_at from now, as if the schedule + // were freshly created. Otherwise a schedule paused while + // next_run_at was still in the future, then re-enabled after that + // date has passed, would fire immediately on the next cron tick + // instead of waiting for its next natural occurrence. + $nextRun = $this->calculateNextRunTime( + $recurring['schedule_type'], + $recurring['schedule_day'], + $recurring['schedule_time'] + ); + $sql = "UPDATE recurring_tickets SET is_active = ?, next_run_at = ? WHERE recurring_id = ?"; + $stmt = $this->conn->prepare($sql); + $stmt->bind_param('isi', $newActive, $nextRun, $recurringId); + } else { + $sql = "UPDATE recurring_tickets SET is_active = ? WHERE recurring_id = ?"; + $stmt = $this->conn->prepare($sql); + $stmt->bind_param('ii', $newActive, $recurringId); + } + $success = $stmt->execute(); $stmt->close(); return ['success' => $success]; From f59b3d529b31de9487b285decf39007f5c4a6045 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:53:25 -0400 Subject: [PATCH 14/29] Notification bell: pause polling when tab hidden, backoff on failure (#59) setInterval(loadNotifications, 60000) ran unconditionally regardless of tab visibility, and failures retried at the same fixed 60s cadence forever. Now skips polling while document.hidden, resumes immediately via visibilitychange when the tab regains focus, and backs off exponentially (capped at 5 min) on repeated fetch failures, resetting to the normal 60s cadence on the next success. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- views/layout_footer.php | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/views/layout_footer.php b/views/layout_footer.php index e10a1ce..144b561 100644 --- a/views/layout_footer.php +++ b/views/layout_footer.php @@ -235,11 +235,12 @@ } function loadNotifications() { - fetch('/api/notifications.php', { credentials: 'same-origin' }) + return fetch('/api/notifications.php', { credentials: 'same-origin' }) .then(function(r) { return r.json(); }) - .then(renderNotifications) + .then(function(data) { renderNotifications(data); return true; }) .catch(function() { list.innerHTML = '
Could not load
'; + return false; }); } @@ -261,9 +262,34 @@ document.addEventListener('click', function(e) { if (_open && wrapEl && !wrapEl.contains(e.target)) closePanel(); }); document.addEventListener('keydown', function(e) { if (e.key === 'Escape' && _open) closePanel(); }); - // Initial badge count + poll every 60s + // Poll every 60s while the tab is visible, backing off (up to 5 min) on + // repeated failures, and resuming immediately when the tab regains focus. + var POLL_INTERVAL = 60000; + var MAX_POLL_INTERVAL = 300000; + var _pollTimer = null; + var _failCount = 0; + + function scheduleNextPoll(delay) { + clearTimeout(_pollTimer); + _pollTimer = setTimeout(pollNotifications, delay); + } + + function pollNotifications() { + if (document.hidden) return; + loadNotifications().then(function(ok) { + _failCount = ok ? 0 : _failCount + 1; + var delay = ok ? POLL_INTERVAL : Math.min(POLL_INTERVAL * Math.pow(2, _failCount), MAX_POLL_INTERVAL); + scheduleNextPoll(delay); + }); + } + + document.addEventListener('visibilitychange', function() { + if (!document.hidden) pollNotifications(); + }); + + // Initial badge count, then start the poll cycle loadNotifications(); - setInterval(loadNotifications, 60000); + scheduleNextPoll(POLL_INTERVAL); })(); From 92aea89b7456f3ea008be8cbf20f9d2e54af051b Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:53:31 -0400 Subject: [PATCH 15/29] User Activity report: filter 'Last Activity' by the selected date range (#52) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last_activity subquery had no WHERE clause on the report's date-range filter, so it always showed true all-time last activity even when the page was filtered to e.g. "last 7 days" — inconsistent with every other column on the same report. Added the same DATE(created_at) BETWEEN ? AND ? clause used by the report's other subqueries. Verified against a local MariaDB instance: an out-of-range audit_log row is correctly excluded from last_activity once the filter is applied. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- index.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/index.php b/index.php index 119f68f..d5982c0 100644 --- a/index.php +++ b/index.php @@ -391,13 +391,16 @@ switch (true) { LEFT JOIN ( SELECT user_id, MAX(created_at) as last_activity FROM audit_log + WHERE DATE(created_at) BETWEEN ? AND ? GROUP BY user_id ) al ON u.user_id = al.user_id ORDER BY tickets_created DESC, tickets_resolved DESC"; $stmt = $conn->prepare($sql); $stmt->bind_param( - 'ssssssss', + 'ssssssssss', + $dateRange['from'], + $dateRange['to'], $dateRange['from'], $dateRange['to'], $dateRange['from'], From 0d6b08f5d288e61e2007a69636075fa34da2ec1f Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:53:36 -0400 Subject: [PATCH 16/29] Cap get_users.php result set as defense-in-depth (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit api/get_users.php returned every user's user_id/username/display_name to any authenticated session with no pagination or limit — needed for mention/assignment typeahead, but a blanket enumeration a compromised low-privilege session could scrape in one call. Added a LIMIT 500; every caller already only uses this for typeahead/dropdown filtering, never a literal full roster, so this doesn't change behavior for any real deployment size while bounding the response. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- api/get_users.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/api/get_users.php b/api/get_users.php index 77e0afd..5971ee3 100644 --- a/api/get_users.php +++ b/api/get_users.php @@ -8,8 +8,10 @@ require_once __DIR__ . '/bootstrap.php'; try { - // Get all users for mentions/assignment - $result = Database::query("SELECT user_id, username, display_name FROM users ORDER BY display_name, username"); + // Get all users for mentions/assignment. Capped as defense-in-depth against + // a single call scraping an unbounded user list — every caller only needs + // this for typeahead/dropdown filtering, never a literal full roster. + $result = Database::query("SELECT user_id, username, display_name FROM users ORDER BY display_name, username LIMIT 500"); if (!$result) { throw new Exception("Failed to query users"); From 3221ccfd29e08d2807c0269db4a372b5409710b4 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:53:42 -0400 Subject: [PATCH 17/29] Batch the audit log retention DELETE to bound lock hold time (#66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deleteOldLogs() ran a single unbounded DELETE. created_at is indexed so row selection itself is cheap, but on a large qualifying set (first run after enabling/changing AUDIT_LOG_RETENTION_DAYS, or after the cron silently missed runs) an unbounded single-statement DELETE holds row locks for the full duration — risking contention with the frequent concurrent INSERTs the audit log receives from live traffic. Now deletes in batches of 1000 (parameterized), looping until nothing qualifies. Verified against a local MariaDB instance with a batch size of 10 forcing multiple loop iterations: deleted exactly the stale rows, left recent rows untouched, correct total count returned. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- models/AuditLogModel.php | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/models/AuditLogModel.php b/models/AuditLogModel.php index f0eea92..dd691cc 100644 --- a/models/AuditLogModel.php +++ b/models/AuditLogModel.php @@ -309,17 +309,28 @@ class AuditLogModel * @param int $daysToKeep Number of days of logs to keep * @return int Number of deleted records */ - public function deleteOldLogs($daysToKeep = 90) + public function deleteOldLogs($daysToKeep = 90, $batchSize = 1000) { + // Batched to bound how long each statement holds row locks — an + // unbounded single DELETE on a large backlog (e.g. the first run after + // enabling/changing retention, or after the cron silently missed runs) + // would otherwise contend with the frequent concurrent INSERTs the + // audit log receives from live traffic. $stmt = $this->conn->prepare( - "DELETE FROM audit_log WHERE created_at < DATE_SUB(NOW(), INTERVAL ? DAY)" + "DELETE FROM audit_log WHERE created_at < DATE_SUB(NOW(), INTERVAL ? DAY) ORDER BY audit_id LIMIT ?" ); - $stmt->bind_param("i", $daysToKeep); - $stmt->execute(); - $affectedRows = $stmt->affected_rows; + $stmt->bind_param("ii", $daysToKeep, $batchSize); + + $totalDeleted = 0; + do { + $stmt->execute(); + $affected = $stmt->affected_rows; + $totalDeleted += $affected; + } while ($affected > 0); + $stmt->close(); - return $affectedRows; + return $totalDeleted; } /** From 700048337f3a1f4c26b40ad23b360f14d0e4eea1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:56:33 -0400 Subject: [PATCH 18/29] Fix inconsistent FK ON DELETE behavior on bulk_operations/ticket_templates (#40) bulk_operations.performed_by and ticket_templates.created_by had no ON DELETE clause (defaulting to RESTRICT), unlike every other user-reference FK in the schema (tickets.*, ticket_attachments, ticket_dependencies, recurring_tickets, api_keys), which all use SET NULL. Deleting a user who ever ran a bulk operation or created a template hard-failed at the DB level instead of nulling the reference, breaking the pattern used everywhere else. performed_by was NOT NULL, so it had to become nullable to support SET NULL, matching how every other SET NULL column is defined. - Fixed 000_baseline.sql for fresh installs. - Added 003_fk_on_delete_set_null.sql for existing deployments. Verified against a local MariaDB instance: reproduced the old RESTRICT schema, ran the migration (twice, for idempotency), then confirmed deleting a user with rows in both tables now nulls the references instead of failing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- migrations/000_baseline.sql | 6 ++--- migrations/003_fk_on_delete_set_null.sql | 32 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 migrations/003_fk_on_delete_set_null.sql diff --git a/migrations/000_baseline.sql b/migrations/000_baseline.sql index 3eeee7d..f5ffec9 100644 --- a/migrations/000_baseline.sql +++ b/migrations/000_baseline.sql @@ -57,7 +57,7 @@ CREATE TABLE IF NOT EXISTS `bulk_operations` ( `operation_id` int(11) NOT NULL AUTO_INCREMENT, `operation_type` varchar(50) NOT NULL, `ticket_ids` text NOT NULL, - `performed_by` int(11) NOT NULL, + `performed_by` int(11) DEFAULT NULL, `parameters` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`parameters`)), -- 32, not 20: 'completed_with_errors' is 21 chars (see 001_widen_bulk_operations_status.sql) `status` varchar(32) DEFAULT 'pending', @@ -69,7 +69,7 @@ CREATE TABLE IF NOT EXISTS `bulk_operations` ( PRIMARY KEY (`operation_id`), KEY `idx_performed_by` (`performed_by`), KEY `idx_created_at` (`created_at`), - CONSTRAINT `bulk_operations_ibfk_1` FOREIGN KEY (`performed_by`) REFERENCES `users` (`user_id`) + CONSTRAINT `bulk_operations_ibfk_1` FOREIGN KEY (`performed_by`) REFERENCES `users` (`user_id`) ON DELETE SET NULL ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; -- ============ custom_field_definitions ============ @@ -238,7 +238,7 @@ CREATE TABLE IF NOT EXISTS `ticket_templates` ( PRIMARY KEY (`template_id`), KEY `created_by` (`created_by`), KEY `idx_template_name` (`template_name`), - CONSTRAINT `ticket_templates_ibfk_1` FOREIGN KEY (`created_by`) REFERENCES `users` (`user_id`) + CONSTRAINT `ticket_templates_ibfk_1` FOREIGN KEY (`created_by`) REFERENCES `users` (`user_id`) ON DELETE SET NULL ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; -- ============ ticket_watchers ============ diff --git a/migrations/003_fk_on_delete_set_null.sql b/migrations/003_fk_on_delete_set_null.sql new file mode 100644 index 0000000..9e8021b --- /dev/null +++ b/migrations/003_fk_on_delete_set_null.sql @@ -0,0 +1,32 @@ +-- Fix inconsistent FK ON DELETE behavior on bulk_operations.performed_by and +-- ticket_templates.created_by +-- +-- Every other user-reference FK in the schema (tickets.created_by/updated_by/ +-- assigned_to, ticket_attachments.uploaded_by, ticket_dependencies.created_by, +-- recurring_tickets.created_by/assigned_to, api_keys.created_by, etc.) uses +-- ON DELETE SET NULL. These two had no ON DELETE clause at all, which +-- defaults to RESTRICT — so deleting a user who ever ran a bulk operation or +-- created a template hard-fails at the DB level instead of nulling the +-- reference, breaking the pattern used everywhere else and potentially +-- blocking legitimate user offboarding/cleanup. +-- +-- bulk_operations.performed_by is NOT NULL today; it must become nullable to +-- support SET NULL, matching how every other SET NULL column in the schema +-- is defined. +-- +-- Safe to re-run. + +ALTER TABLE `bulk_operations` + MODIFY COLUMN `performed_by` int(11) DEFAULT NULL; + +ALTER TABLE `bulk_operations` + DROP FOREIGN KEY IF EXISTS `bulk_operations_ibfk_1`; + +ALTER TABLE `bulk_operations` + ADD CONSTRAINT `bulk_operations_ibfk_1` FOREIGN KEY (`performed_by`) REFERENCES `users` (`user_id`) ON DELETE SET NULL; + +ALTER TABLE `ticket_templates` + DROP FOREIGN KEY IF EXISTS `ticket_templates_ibfk_1`; + +ALTER TABLE `ticket_templates` + ADD CONSTRAINT `ticket_templates_ibfk_1` FOREIGN KEY (`created_by`) REFERENCES `users` (`user_id`) ON DELETE SET NULL; From 1e972fe7dce4e97b0cafa146d42129367c93e428 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:58:20 -0400 Subject: [PATCH 19/29] Add memory_limit/max_execution_time sanity checks (#106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config/requirements.php only checked PHP version and 6 extensions. A deployment on a host with a low default memory_limit (e.g. shared- hosting-style 128M) passed the startup requirements check cleanly and only surfaced as a mysterious failure under real load — a large CSV export, an oversized dashboard query on a big install. Added min_memory_limit_mb (256) and min_max_execution_time (30s) thresholds to config/requirements.php, checked as warnings (not hard failures, since a low limit doesn't break every request) in both scripts/check_requirements.php (CI) and api/health.php (production monitoring). -1/0 (unlimited) always passes. Verified the ini-size parsing and warning logic directly with low/high/unlimited memory_limit and max_execution_time values. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- api/health.php | 33 ++++++++++++++++++++++++ config/requirements.php | 7 +++++ scripts/check_requirements.php | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) diff --git a/api/health.php b/api/health.php index 908eb31..8be48e4 100644 --- a/api/health.php +++ b/api/health.php @@ -129,6 +129,39 @@ if (version_compare(PHP_VERSION, $requirements['min_php_version'], '>=')) { $healthy = false; } +// Check 7: memory_limit / max_execution_time sanity (warnings, not fatal — a +// low default doesn't fail requests until something large actually runs, so +// surface it here rather than waiting for a mysterious failure under load). +$memLimitIni = ini_get('memory_limit'); +$memLimitUnit = strtolower(substr(trim($memLimitIni), -1)); +$memLimitBytes = $memLimitIni === '-1' + ? -1 + : (int)$memLimitIni * match ($memLimitUnit) { + 'g' => 1024 * 1024 * 1024, + 'm' => 1024 * 1024, + 'k' => 1024, + default => 1, + }; +$minMemBytes = $requirements['min_memory_limit_mb'] * 1024 * 1024; +if ($memLimitBytes === -1 || $memLimitBytes >= $minMemBytes) { + $checks['memory_limit'] = ['status' => 'ok', 'message' => $memLimitIni]; +} else { + $checks['memory_limit'] = [ + 'status' => 'warning', + 'message' => sprintf('%s is below the recommended minimum %dM', $memLimitIni, $requirements['min_memory_limit_mb']) + ]; +} + +$maxExecTime = (int)ini_get('max_execution_time'); +if ($maxExecTime === 0 || $maxExecTime >= $requirements['min_max_execution_time']) { + $checks['max_execution_time'] = ['status' => 'ok', 'message' => (string)$maxExecTime]; +} else { + $checks['max_execution_time'] = [ + 'status' => 'warning', + 'message' => sprintf('%ds is below the recommended minimum %ds', $maxExecTime, $requirements['min_max_execution_time']) + ]; +} + // Calculate response time $responseTime = round((microtime(true) - $startTime) * 1000, 2); diff --git a/config/requirements.php b/config/requirements.php index d44c6ba..68784f0 100644 --- a/config/requirements.php +++ b/config/requirements.php @@ -25,4 +25,11 @@ return [ 'fileinfo', // api/upload_attachment.php — MIME validation 'json', // request/response encoding (bundled, but assert anyway) ], + + // Sanity-check thresholds (warnings, not hard failures). A host with a low + // default memory_limit passes a bare extension/version check cleanly and + // only surfaces as a mysterious failure under real load — a large CSV + // export, an oversized dashboard query on a big install. + 'min_memory_limit_mb' => 256, + 'min_max_execution_time' => 30, // seconds; 0 (unlimited) always passes ]; diff --git a/scripts/check_requirements.php b/scripts/check_requirements.php index 3e60ffb..c0f48c8 100644 --- a/scripts/check_requirements.php +++ b/scripts/check_requirements.php @@ -10,9 +10,30 @@ * Usage: php scripts/check_requirements.php */ +/** + * Parse a php.ini size value (e.g. "128M", "1G", "-1") into bytes. + * Returns -1 for unlimited. + */ +function parseIniBytes(string $val): int +{ + $val = trim($val); + if ($val === '' || $val === '-1') { + return -1; + } + $unit = strtolower(substr($val, -1)); + $num = (int)$val; + return match ($unit) { + 'g' => $num * 1024 * 1024 * 1024, + 'm' => $num * 1024 * 1024, + 'k' => $num * 1024, + default => $num, + }; +} + $req = require __DIR__ . '/../config/requirements.php'; $errors = []; +$warnings = []; // PHP version $minPhp = $req['min_php_version']; @@ -27,6 +48,28 @@ foreach ($req['required_extensions'] as $ext) { } } +// memory_limit / max_execution_time sanity checks (warnings, not hard +// failures — see config/requirements.php for why these matter). +$memLimitIni = ini_get('memory_limit'); +$memLimitBytes = parseIniBytes($memLimitIni); +$minMemBytes = $req['min_memory_limit_mb'] * 1024 * 1024; +if ($memLimitBytes !== -1 && $memLimitBytes < $minMemBytes) { + $warnings[] = sprintf( + 'memory_limit is %s, below the recommended minimum %dM', + $memLimitIni, + $req['min_memory_limit_mb'] + ); +} + +$maxExecTime = (int)ini_get('max_execution_time'); +if ($maxExecTime !== 0 && $maxExecTime < $req['min_max_execution_time']) { + $warnings[] = sprintf( + 'max_execution_time is %ds, below the recommended minimum %ds', + $maxExecTime, + $req['min_max_execution_time'] + ); +} + if (!empty($errors)) { fwrite(STDERR, "Requirement check FAILED:\n"); foreach ($errors as $err) { @@ -35,6 +78,10 @@ if (!empty($errors)) { exit(1); } +foreach ($warnings as $warn) { + fwrite(STDERR, "Requirement check WARNING: " . $warn . "\n"); +} + printf( "Requirement check passed: PHP %s (>= %s); extensions: %s\n", PHP_VERSION, From 70ef42c3116ab84108e92bad1201e7e7de78678d Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:58:58 -0400 Subject: [PATCH 20/29] Delete dead helpers/OutputHelper.php (#54) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero callers anywhere in the app — confirmed via grep for "OutputHelper::" across the whole codebase. Every view actually calls htmlspecialchars() directly instead, which a prior audit confirmed is done consistently, so escaping was never actually at risk. This was just a misleading, unused class that README.md's file reference implied was part of the app's active XSS-prevention story. Removed the file and its README Project Structure entry. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- README.md | 1 - helpers/OutputHelper.php | 212 --------------------------------------- 2 files changed, 213 deletions(-) delete mode 100644 helpers/OutputHelper.php diff --git a/README.md b/README.md index 3f0ce79..c86480c 100644 --- a/README.md +++ b/README.md @@ -362,7 +362,6 @@ tinker_tickets/ │ ├── Database.php # Centralized mysqli connection │ ├── ErrorHandler.php # Global error/exception handler │ ├── NotificationHelper.php # Matrix hookshot webhook events -│ ├── OutputHelper.php # Safe HTML output helpers │ ├── ResponseHelper.php # JSON API response helpers │ ├── SynapseHelper.php # Resolves usernames → Matrix IDs via Synapse admin API │ └── UrlHelper.php # Canonical ticket URLs using APP_DOMAIN diff --git a/helpers/OutputHelper.php b/helpers/OutputHelper.php deleted file mode 100644 index 54f7201..0000000 --- a/helpers/OutputHelper.php +++ /dev/null @@ -1,212 +0,0 @@ -

- * - * @param string|null $string The string to escape - * @param int $flags htmlspecialchars flags (default: ENT_QUOTES | ENT_HTML5) - * @return string Escaped string - */ - public static function h(?string $string, int $flags = ENT_QUOTES | ENT_HTML5): string - { - if ($string === null) { - return ''; - } - return htmlspecialchars($string, $flags, 'UTF-8'); - } - - /** - * Escape string for HTML attribute context - * - * Use for values inside HTML attributes. - * Example: - * - * @param string|null $string The string to escape - * @return string Escaped string - */ - public static function attr(?string $string): string - { - if ($string === null) { - return ''; - } - // More aggressive escaping for attribute context - return htmlspecialchars($string, ENT_QUOTES | ENT_HTML5 | ENT_SUBSTITUTE, 'UTF-8'); - } - - /** - * Encode data as JSON for JavaScript context - * - * Use when embedding data in JavaScript. - * Example: - * - * @param mixed $data The data to encode - * @param int $flags json_encode flags - * @return string JSON encoded string (safe for script context) - */ - public static function json($data, int $flags = 0): string - { - // Use HEX encoding for safety in HTML context - $safeFlags = JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_QUOT | JSON_HEX_AMP | $flags; - return json_encode($data, $safeFlags); - } - - /** - * URL encode a string - * - * Use for values in URL query strings. - * Example: - * - * @param string|null $string The string to encode - * @return string URL encoded string - */ - public static function url(?string $string): string - { - if ($string === null) { - return ''; - } - return rawurlencode($string); - } - - /** - * Escape for CSS context - * - * Use for values in inline CSS. - * Example:
- * - * @param string|null $string The string to escape - * @return string Escaped string (only allows safe characters) - */ - public static function css(?string $string): string - { - if ($string === null) { - return ''; - } - // Only allow alphanumeric, hyphens, underscores, spaces, and common CSS values - if (!preg_match('/^[a-zA-Z0-9_\-\s#.,()%]+$/', $string)) { - return ''; - } - return $string; - } - - /** - * Format a number safely - * - * Ensures output is always a valid number. - * - * @param mixed $number The number to format - * @param int $decimals Number of decimal places - * @return string Formatted number - */ - public static function number($number, int $decimals = 0): string - { - return number_format((float)$number, $decimals, '.', ','); - } - - /** - * Format an integer safely - * - * @param mixed $value The value to format - * @return int Integer value - */ - public static function int($value): int - { - return (int)$value; - } - - /** - * Truncate string with ellipsis - * - * @param string|null $string The string to truncate - * @param int $length Maximum length - * @param string $suffix Suffix to add if truncated - * @return string Truncated and escaped string - */ - public static function truncate(?string $string, int $length = 100, string $suffix = '...'): string - { - if ($string === null) { - return ''; - } - - if (mb_strlen($string, 'UTF-8') <= $length) { - return self::h($string); - } - - return self::h(mb_substr($string, 0, $length, 'UTF-8')) . self::h($suffix); - } - - /** - * Format a date safely - * - * @param string|int|null $date Date string, timestamp, or null - * @param string $format PHP date format - * @return string Formatted date - */ - public static function date($date, string $format = 'Y-m-d H:i:s'): string - { - if ($date === null || $date === '') { - return ''; - } - - if (is_numeric($date)) { - return date($format, (int)$date); - } - - $timestamp = strtotime($date); - if ($timestamp === false) { - return ''; - } - - return date($format, $timestamp); - } - - /** - * Check if a string is safe for use as a CSS class name - * - * @param string $class The class name to validate - * @return bool True if safe - */ - public static function isValidCssClass(string $class): bool - { - return preg_match('/^[a-zA-Z_][a-zA-Z0-9_-]*$/', $class) === 1; - } - - /** - * Sanitize CSS class name(s) - * - * @param string|null $classes Space-separated class names - * @return string Sanitized class names - */ - public static function cssClass(?string $classes): string - { - if ($classes === null || $classes === '') { - return ''; - } - - $classList = explode(' ', $classes); - $validClasses = array_filter($classList, [self::class, 'isValidCssClass']); - - return implode(' ', $validClasses); - } -} - -/** - * Shorthand function for HTML escaping - * - * @param string|null $string The string to escape - * @return string Escaped string - */ -function h(?string $string): string -{ - return OutputHelper::h($string); -} From caeb9269d943b7cb9fe34de5f9c561abe0232f69 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 11:59:23 -0400 Subject: [PATCH 21/29] README: add missing StatsModel::invalidateCache() caller (#92) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dev Note #24 listed 7 callers; api/ticket_status_api.php (the Bearer API's status-change endpoint) also correctly calls invalidateCache() but wasn't in the list. Behavior was already correct — this is a pure documentation completeness fix so the caller list stays an accurate reference for future maintainers deciding whether a new mutating path needs the same call. Verified via grep -rl "invalidateCache" that these 8 files (plus StatsModel.php itself, and an unrelated same-named method on UserModel) are the complete set of real callers. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index c86480c..ac755b4 100644 --- a/README.md +++ b/README.md @@ -555,7 +555,7 @@ Key conventions and gotchas for working with this codebase: 21. **Confirm dialogs**: Never use browser `confirm()`. Use `showConfirmModal(title, message, type, onConfirm)` (defined in `utils.js`, available on all pages). Types: `'warning'` | `'error'` | `'info'`. 22. **`utils.js` on all pages**: `utils.js` is loaded by all views (including admin). It provides `escapeHtml()`, `getTicketIdFromUrl()`, and `showConfirmModal()`. 23. **No `toast.js`**: `toast.js` is deprecated and no longer loaded by any view. Use `lt.toast.success/error/warning/info()` directly from `base.js`. -24. **Stats cache**: `StatsModel` caches stats for 60 s. Any path that modifies ticket state must call `(new StatsModel($conn))->invalidateCache()` after the change. Callers: `TicketController::create` (manual create), `create_ticket_api.php` (external API create/escalate/reopen), `cron/create_recurring_tickets.php`, `bulk_operation`, `assign_ticket`, `update_ticket`, and `clone_ticket`. +24. **Stats cache**: `StatsModel` caches stats for 60 s. Any path that modifies ticket state must call `(new StatsModel($conn))->invalidateCache()` after the change. Callers: `TicketController::create` (manual create), `create_ticket_api.php` (external API create/escalate/reopen), `cron/create_recurring_tickets.php`, `bulk_operation`, `assign_ticket`, `update_ticket`, `clone_ticket`, and `ticket_status_api.php` (Bearer API status-change endpoint). 25. **External API (`create_ticket_api.php`)**: Uses `ApiKeyAuth` (Bearer token), not session auth. Served directly by the web server from the document root — not through the index.php router. Includes deduplication logic (SHA-256 hash, no time window) that updates/escalates an existing open duplicate or reopens a closed one rather than creating a new ticket. ## File Reference From 3f1e06479de4f1886708345b78e2e8878d602657 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 12:22:02 -0400 Subject: [PATCH 22/29] Strip EXIF/GPS metadata from image uploads (#97) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit api/upload_attachment.php did a raw move_uploaded_file() with zero image processing. A photo attached from a phone retained embedded EXIF, including GPS coordinates, and download_attachment.php streams the file byte-for-byte back to any user with ticket visibility — for an infrastructure company, this could leak a data center or office's precise physical location through a routine ticket photo, especially on Confidential-visibility tickets whose whole point is restricting exactly this kind of detail. Added stripImageMetadata(): decodes and re-encodes JPEG/PNG/GIF/WebP uploads via GD, which drops EXIF chunks that aren't part of the pixel data. Best-effort — leaves the file untouched on any failure (corrupt image, unsupported format, GD unavailable, or an oversized decoded pixel count guarding against a decompression-bomb-style crafted image) rather than blocking the upload. Verified with a real GPS-tagged JPEG (generated via piexif) and a GD/PHP harness: GPS EXIF is gone after stripping, the image stays valid and correctly sized, PNG alpha transparency is preserved, and corrupt files / non-image MIME types are left byte-for-byte unchanged. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- api/upload_attachment.php | 72 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/api/upload_attachment.php b/api/upload_attachment.php index 93edf0d..6fa4e48 100644 --- a/api/upload_attachment.php +++ b/api/upload_attachment.php @@ -29,6 +29,73 @@ require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php'; header('Content-Type: application/json'); +/** + * Strip EXIF/metadata (including GPS) from an image file in place by + * decoding and re-encoding it via GD, which drops metadata chunks that + * aren't part of the pixel data. Best-effort: leaves the file untouched on + * any failure (corrupt image, unsupported format, GD unavailable) rather + * than blocking the upload — original bytes are what would have been stored + * anyway before this existed. + * + * download_attachment.php streams attachments back byte-for-byte to any user + * with ticket visibility, so an unstripped phone photo's embedded GPS data + * would otherwise leak a data center/office's physical location even on a + * Confidential-visibility ticket. + */ +function stripImageMetadata(string $path, string $mimeType): void +{ + if (!extension_loaded('gd')) { + return; + } + + // Guard against a decompression-bomb-style crafted image (small file, + // huge decoded pixel buffer) exhausting memory during decode. + $dims = @getimagesize($path); + if ($dims === false) { + return; + } + [$width, $height] = $dims; + if ($width * $height > 40_000_000) { // ~40 MP cap + return; + } + + $loaders = [ + 'image/jpeg' => 'imagecreatefromjpeg', + 'image/png' => 'imagecreatefrompng', + 'image/gif' => 'imagecreatefromgif', + 'image/webp' => 'imagecreatefromwebp', + ]; + $loader = $loaders[$mimeType] ?? null; + if ($loader === null || !function_exists($loader)) { + return; + } + + $image = @$loader($path); + if ($image === false) { + return; + } + + // Preserve transparency for formats that support it. + imagesavealpha($image, true); + imagealphablending($image, false); + + $tmpPath = $path . '.tmp'; + $saved = match ($mimeType) { + 'image/jpeg' => imagejpeg($image, $tmpPath, 90), + 'image/png' => imagepng($image, $tmpPath, 6), + 'image/gif' => imagegif($image, $tmpPath), + 'image/webp' => imagewebp($image, $tmpPath, 90), + default => false, + }; + imagedestroy($image); + + if ($saved && file_exists($tmpPath)) { + rename($tmpPath, $path); + } elseif (file_exists($tmpPath)) { + unlink($tmpPath); + } +} + // Check authentication if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) { ResponseHelper::unauthorized(); @@ -184,6 +251,11 @@ if (!move_uploaded_file($file['tmp_name'], $targetPath)) { ResponseHelper::serverError('Failed to move uploaded file'); } +// Strip EXIF/GPS metadata from image uploads before it's ever served back +if (str_starts_with($mimeType, 'image/')) { + stripImageMetadata($targetPath, $mimeType); +} + // Sanitize original filename $originalFilename = basename($file['name']); $originalFilename = preg_replace('/[^\w\s\-\.]/', '', $originalFilename); From 4fade1a9d3c378303b91236ed434160635b7d6cb Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 12:23:04 -0400 Subject: [PATCH 23/29] Reject the semantic inverse of an existing ticket dependency (#51) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addDependency()'s "already exists" check only matched the exact (ticket_id, depends_on_id, dependency_type) tuple. A user could add "A blocks B" from ticket A's page, then separately add "B blocked_by A" from ticket B's page — wouldCreateCycle() correctly found no cycle (both normalize to the same precedence edge), so the insert was allowed, creating two DB rows describing one real relationship (shown twice on ticket B's page: once under Dependencies, once under Dependents). Added an inverse-relationship check before the insert: blocks/ blocked_by are inverses of each other, relates_to is its own inverse (symmetric). duplicates has no defined inverse type in the schema, so both directions remain independently insertable, which is correct — "A duplicates B" and "B duplicates A" are distinct claims. Verified against a local MariaDB instance: the exact repro from the issue (A blocks B, then B blocked_by A) is now rejected, relates_to's symmetric case is rejected in both directions, and duplicates in either direction is unaffected. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- models/DependencyModel.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/models/DependencyModel.php b/models/DependencyModel.php index a7caf31..8f37899 100644 --- a/models/DependencyModel.php +++ b/models/DependencyModel.php @@ -172,6 +172,27 @@ class DependencyModel } $checkStmt->close(); + // Also check the semantic inverse: "A blocks B" and "B blocked_by A" + // describe the same relationship, so adding one from either ticket's + // page must be rejected as a duplicate of the other. relates_to is + // its own inverse (symmetric); duplicates has no defined inverse type. + $inverseTypes = ['blocks' => 'blocked_by', 'blocked_by' => 'blocks', 'relates_to' => 'relates_to']; + if (isset($inverseTypes[$type])) { + $inverseType = $inverseTypes[$type]; + $checkInverseSql = "SELECT dependency_id FROM ticket_dependencies + WHERE ticket_id = ? AND depends_on_id = ? AND dependency_type = ?"; + $checkInverseStmt = $this->conn->prepare($checkInverseSql); + $checkInverseStmt->bind_param("sss", $dependsOnId, $ticketId, $inverseType); + $checkInverseStmt->execute(); + $inverseResult = $checkInverseStmt->get_result(); + + if ($inverseResult->num_rows > 0) { + $checkInverseStmt->close(); + return ['success' => false, 'error' => 'This relationship already exists']; + } + $checkInverseStmt->close(); + } + // Check for circular dependency if ($this->wouldCreateCycle($ticketId, $dependsOnId, $type)) { return ['success' => false, 'error' => 'This would create a circular dependency']; From 6553c0227d44d79c3db1840ffbae16116acfe552 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 12:25:04 -0400 Subject: [PATCH 24/29] Fix dogpile cache-overwrite race in CacheHelper::remember() (#91) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remember() had no protection against a slow cache-miss recomputation overwriting a fresher write. If Request A started computing stats just before a ticket mutation + invalidateCache(), and Request B started just after (correctly computing fresh, post-mutation data), A could finish (using stale pre-mutation data) after B and overwrite B's fresh cache entry — extending staleness by up to another full TTL. Added a per-prefix invalidation epoch: delete() bumps it, and remember() snapshots it before running the callback and only writes if the epoch hasn't changed since — otherwise a newer invalidation happened mid-computation and the result being written is already stale, so it's dropped (the caller still gets its own result; only the cache write is skipped). Verified with two real concurrent PHP processes racing against the same cache key (a slow "Request A" callback vs. a fast "Request B" that invalidates then recomputes): the cache ends up holding B's fresh value, not A's late stale overwrite. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- helpers/CacheHelper.php | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/helpers/CacheHelper.php b/helpers/CacheHelper.php index 40c1c83..4300adf 100644 --- a/helpers/CacheHelper.php +++ b/helpers/CacheHelper.php @@ -121,6 +121,33 @@ class CacheHelper return $written; } + /** + * Read the current invalidation epoch for a prefix (0 if never bumped). + * Used by remember() to detect an invalidation that happened while a + * cache-miss recomputation was in flight. + */ + private static function getEpoch(string $prefix): int + { + $safePrefix = preg_replace('/[^a-zA-Z0-9_]/', '_', $prefix); + $file = self::getCacheDir() . '/' . $safePrefix . '.epoch'; + $val = @file_get_contents($file); + return $val !== false ? (int)$val : 0; + } + + /** + * Bump a prefix's invalidation epoch. Called whenever anything under the + * prefix is invalidated. + */ + private static function bumpEpoch(string $prefix): void + { + $safePrefix = preg_replace('/[^a-zA-Z0-9_]/', '_', $prefix); + $file = self::getCacheDir() . '/' . $safePrefix . '.epoch'; + $next = self::getEpoch($prefix) + 1; + if (@file_put_contents($file, (string)$next, LOCK_EX) !== false) { + @chmod($file, 0600); + } + } + /** * Delete cached data * @@ -130,6 +157,8 @@ class CacheHelper */ public static function delete(string $prefix, $identifier = null): bool { + self::bumpEpoch($prefix); + if ($identifier !== null) { $key = self::makeKey($prefix, $identifier); unset(self::$memoryCache[$key]); @@ -192,8 +221,14 @@ class CacheHelper $data = self::get($prefix, $identifier, $ttl); if ($data === null) { + // Snapshot the epoch before running the (possibly slow) callback so + // a concurrent invalidation mid-computation can be detected below — + // otherwise this request's stale pre-invalidation result could + // overwrite a newer request's fresher write, extending staleness by + // up to another full TTL. + $epochBefore = self::getEpoch($prefix); $data = $callback(); - if ($data !== null) { + if ($data !== null && self::getEpoch($prefix) === $epochBefore) { self::set($prefix, $identifier, $data); } } From e4c240009d7f6c376a7e96cf1d54dcdf7ea3c451 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 12:27:26 -0400 Subject: [PATCH 25/29] Dashboard cleanup: chart empty-state message, dead click-handler (#101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small findings from the same dashboard audit pass: 1. Charts rendered a bare empty frame with no message when the filtered dataset was empty (fresh install, or a non-admin's visibility-filtered ticket set happening to be zero). makeDonut/ makeBar now show a "No data for current filters" message in the chart's place instead of silently doing nothing. 2. Stat cards had two independent, redundant click-handler implementations. lt.statsFilter.init() (base.js, shared web_template code) read each card's data-filter-key/data-filter-val attributes and called window.lt_onStatFilter(key, val) on click — but that global is never defined anywhere in this app, so it only toggled a cosmetic .active class with no functional effect. The actual navigation logic is the separate handler at ~line 1282 that ignores those attributes entirely. Both fired on the same click with no visible symptom, but the markup looked load-bearing and wasn't — a trap for a future edit that touches one implementation assuming it's the only one. Removed the dead lt.statsFilter.init() call and the now-unused data-filter-key/data-filter-val attributes from this app's DashboardView.php (left the shared lt.statsFilter module in base.js itself untouched, since other LotusGuild apps consuming the same shared template file may define their own lt_onStatFilter). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- views/DashboardView.php | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/views/DashboardView.php b/views/DashboardView.php index 2db7bed..4417744 100644 --- a/views/DashboardView.php +++ b/views/DashboardView.php @@ -120,7 +120,6 @@ include __DIR__ . '/layout_header.php'; ?>
[ # ]
@@ -133,7 +132,6 @@ include __DIR__ . '/layout_header.php';
[ ! ]
@@ -146,7 +144,6 @@ include __DIR__ . '/layout_header.php';
[ @ ]
@@ -171,7 +168,6 @@ include __DIR__ . '/layout_header.php';
[ OK ]
@@ -334,7 +330,8 @@ include __DIR__ . '/layout_header.php'; function makeDonut(canvasId, data, colorMap) { var ctx = document.getElementById(canvasId); - if (!ctx || !data.length) return; + if (!ctx) return; + if (!data.length) { showChartEmptyState(ctx); return; } ctx.title = 'Click a segment to filter the ticket list'; return new Chart(ctx, { type: 'doughnut', @@ -365,9 +362,22 @@ include __DIR__ . '/layout_header.php'; }); } + function showChartEmptyState(canvas) { + canvas.style.display = 'none'; + var wrap = canvas.parentElement; + if (wrap && !wrap.querySelector('.lt-chart-empty')) { + var msg = document.createElement('div'); + msg.className = 'lt-chart-empty'; + msg.style.cssText = 'display:flex;align-items:center;justify-content:center;height:100%;color:var(--text-muted);font-size:0.75rem'; + msg.textContent = 'No data for current filters'; + wrap.appendChild(msg); + } + } + function makeBar(canvasId, data) { var ctx = document.getElementById(canvasId); - if (!ctx || !data.length) return; + if (!ctx) return; + if (!data.length) { showChartEmptyState(ctx); return; } ctx.title = 'Click a bar to filter the ticket list'; return new Chart(ctx, { type: 'bar', @@ -1226,7 +1236,6 @@ window.TICKET_STATUSES = Date: Tue, 8 Sep 2026 14:12:04 -0400 Subject: [PATCH 26/29] Fix .env.example: missing Matrix vars, plus a real parse_ini_file outage (#63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Primary fix (#63): added the 5 env vars config.php reads and README documents but .env.example never listed: MATRIX_DOMAIN, SYNAPSE_ADMIN_URL, SYNAPSE_ADMIN_TOKEN, MATRIX_NOTIFY_COMMENTS, and MATRIX_NOTIFY_ASSIGNMENTS. A deployer following only .env.example had no indication these existed, silently missing watcher Matrix DMs and comment/assignment notifications. While verifying the fix by actually running .env.example through parse_ini_file() (what config.php calls), found this file could not be parsed at all — a real, currently-live outage for anyone following its own first-line instruction ("Copy this file to .env and fill in your values"): 1. PHP's ini parser treats "#" comments as fragile: punctuation like parentheses or quotes inside a "#" comment can throw a syntax error even though the line is meant to be inert. The file's header comment itself (and 15+ other comment lines) tripped this. Switched every comment to ";", which parse_ini_file treats as a true inert comment regardless of content — verified with isolated repros of both prefixes under all three INI_SCANNER_* modes. 2. LDAP_BIND_DN's example value contained unquoted "=" and commas, violating the file's own documented quoting rule and causing a second, independent parse failure. Quoted it (and the two other comma-bearing LDAP DN values) to match the rule. config.php has zero fallback for a parse failure — it die()s immediately — so either bug alone would have taken down every fresh deployment that didn't hand-edit the example file's comments first. Verified end-to-end: copied .env.example to a real .env file unmodified and ran it through config.php's exact parse_ini_file + quote-stripping logic; it now parses cleanly with all 23 keys (including the 5 new ones) and LDAP_BIND_DN resolves to the correct unquoted DN string. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- .env.example | 93 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 37 deletions(-) diff --git a/.env.example b/.env.example index cbce12a..4b76f6f 100644 --- a/.env.example +++ b/.env.example @@ -1,60 +1,79 @@ -# Tinker Tickets Environment Configuration -# Copy this file to .env and fill in your values -# -# NOTE: This file is parsed with parse_ini_file(). Any value containing special -# characters (#, ;, =, quotes, spaces, etc.) MUST be wrapped in double quotes, -# e.g. DB_PASS="p@ss;word#1". The application now fails loudly (dies with a clear -# error) if the .env file cannot be parsed, so an unquoted special character will -# take the whole app down rather than silently using a wrong value. +; Tinker Tickets Environment Configuration +; Copy this file to .env and fill in your values +; +; NOTE: This file is parsed with PHP's parse_ini_file. Any value containing +; special characters -- #, ;, =, quotes, spaces, etc. -- MUST be wrapped in +; double quotes, e.g. DB_PASS="p@ss;word#1". The application now fails loudly +; -- dies with a clear error -- if the .env file cannot be parsed, so an +; unquoted special character will take the whole app down rather than +; silently using a wrong value. +; +; Comments in this file use ";" rather than "#": PHP's ini parser treats "#" +; comments as fragile -- punctuation like parentheses or quotes inside a "#" +; comment can produce a syntax error even though the line is meant to be +; inert, silently breaking every value below it. ";" comments don't have this +; problem, so keep using ";" for any comment added to this file. -# Database Configuration +; Database Configuration DB_HOST=10.10.10.50 DB_USER=tinkertickets DB_PASS=your_password_here DB_NAME=ticketing_system -# Matrix Webhook (optional - for notifications via matrix-hookshot) -# Set to your hookshot generic webhook URL, e.g.: -# https://matrix.lotusguild.org/webhook/ +; Matrix Webhook (optional - for notifications via matrix-hookshot) +; Set to your hookshot generic webhook URL, e.g.: +; https://matrix.lotusguild.org/webhook/uuid-goes-here MATRIX_WEBHOOK_URL= -# Matrix users to @mention on every new ticket (comma-separated Matrix user IDs) -# e.g. @jared:matrix.lotusguild.org,@alice:matrix.lotusguild.org +; Matrix users to @mention on every new ticket (comma-separated Matrix user IDs) +; e.g. @jared:matrix.lotusguild.org,@alice:matrix.lotusguild.org MATRIX_NOTIFY_USERS= -# Application Domain (required for Matrix webhook ticket links) -# Set this to your public domain (e.g., t.lotusguild.org) +; Matrix homeserver domain (used to build Matrix user IDs from LLDAP usernames) +MATRIX_DOMAIN= + +; Synapse internal URL and admin token (used to resolve usernames -> Matrix IDs +; for watcher DMs) +SYNAPSE_ADMIN_URL= +SYNAPSE_ADMIN_TOKEN= + +; Optional: send a Matrix notification on comments and/or assignments (0/1) +MATRIX_NOTIFY_COMMENTS=0 +MATRIX_NOTIFY_ASSIGNMENTS=0 + +; Application Domain (required for Matrix webhook ticket links) +; Set this to your public domain, e.g. t.lotusguild.org APP_DOMAIN= -# Allowed Hosts for HTTP_HOST validation (comma-separated) -# Include all domains that can access this application +; Allowed Hosts for HTTP_HOST validation (comma-separated) +; Include all domains that can access this application ALLOWED_HOSTS=localhost,127.0.0.1 -# Trusted reverse proxy IP(s), comma-separated (e.g. the Authelia/nginx proxy). -# Set this to the IP address(es) of your reverse proxy. Authelia forward-auth -# headers (Remote-User / Remote-Groups) and forwarded client IPs are only -# trusted when REMOTE_ADDR is in this list. -# -# Leaving this EMPTY disables reverse-proxy verification entirely: the app then -# trusts Remote-User / Remote-Groups headers from ANY source. That is unsafe if -# the PHP backend is reachable directly (bypassing the proxy), because a client -# can then spoof those headers and log in as an admin. Only leave it empty when -# network topology guarantees PHP is reachable solely via the trusted proxy. -# -# Exact IP match only (no CIDR). Example (single proxy): TRUSTED_PROXIES=10.10.10.27 -# Example (multiple): TRUSTED_PROXIES=10.10.10.27,10.10.10.28 +; Trusted reverse proxy IPs, comma-separated -- e.g. the Authelia/nginx proxy. +; Set this to the IP address(es) of your reverse proxy. Authelia forward-auth +; headers (Remote-User / Remote-Groups) and forwarded client IPs are only +; trusted when REMOTE_ADDR is in this list. +; +; Leaving this EMPTY disables reverse-proxy verification entirely: the app then +; trusts Remote-User / Remote-Groups headers from ANY source. That is unsafe if +; the PHP backend is reachable directly (bypassing the proxy), because a client +; can then spoof those headers and log in as an admin. Only leave it empty when +; network topology guarantees PHP is reachable solely via the trusted proxy. +; +; Exact IP match only (no CIDR). Example (single proxy): TRUSTED_PROXIES=10.10.10.27 +; Example (multiple): TRUSTED_PROXIES=10.10.10.27,10.10.10.28 TRUSTED_PROXIES= -# Timezone (default: America/New_York) +; Timezone (default: America/New_York) TIMEZONE=America/New_York -# LDAP / lldap (for user avatar lookups) +; LDAP / lldap (for user avatar lookups) LDAP_ENABLED=true LDAP_HOST=10.10.10.39 LDAP_PORT=3890 -LDAP_BIND_DN=uid=tinker-tickets,ou=people,dc=example,dc=com +LDAP_BIND_DN="uid=tinker-tickets,ou=people,dc=example,dc=com" LDAP_BIND_PW= -LDAP_BASE_DN=dc=example,dc=com -LDAP_USER_BASE=ou=people,dc=example,dc=com -# How long to cache avatar images locally (seconds, default 3600) +LDAP_BASE_DN="dc=example,dc=com" +LDAP_USER_BASE="ou=people,dc=example,dc=com" +; How long to cache avatar images locally (seconds, default 3600) AVATAR_CACHE_TTL=3600 From 338bed7eb7e6e1ad1aec7f1f10461e7d0dd0ccc4 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 14:14:00 -0400 Subject: [PATCH 27/29] Add per-ticket attachment count/storage quota (#55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit api/upload_attachment.php enforced a per-file size cap but nothing bounded the total number of attachments on a single ticket or their cumulative size over time — an authenticated low-privilege user could slowly fill the uploads/ disk by attaching many files across tickets, bounded only by the general rate limiter (which throttles request rate, not storage volume). Added MAX_ATTACHMENTS_PER_TICKET (50) and MAX_TOTAL_ATTACHMENT_SIZE_PER_TICKET (100MB) config defaults, enforced before move_uploaded_file() using AttachmentModel::getAttachmentCount() and getTotalSizeForTicket() — both already existed in the model with zero callers, apparently added for exactly this purpose but never wired in. Verified against a local MariaDB instance: with 3 existing 1MB attachments and a 3-attachment cap, the count check correctly rejects a 4th; with a 5MB total cap, a 2.5MB upload that would push the ticket over the limit is correctly rejected while a small one that fits is not. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- api/upload_attachment.php | 18 +++++++++++++++++- config/config.php | 2 ++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/api/upload_attachment.php b/api/upload_attachment.php index 6fa4e48..481d99f 100644 --- a/api/upload_attachment.php +++ b/api/upload_attachment.php @@ -194,6 +194,23 @@ if ($file['size'] > $maxSize) { ResponseHelper::error('File size exceeds maximum allowed (' . AttachmentModel::formatFileSize($maxSize) . ')'); } +// Check per-ticket attachment count/storage quota — bounds an authenticated +// low-privilege user slowly filling the uploads/ disk across many tickets, +// which was previously bounded only by the request-rate limiter, not volume. +$attachmentModel = new AttachmentModel($conn); +$maxAttachments = $GLOBALS['config']['MAX_ATTACHMENTS_PER_TICKET'] ?? 50; +if ($attachmentModel->getAttachmentCount($ticketId) >= $maxAttachments) { + ResponseHelper::error("This ticket already has the maximum of {$maxAttachments} attachments"); +} + +$maxTotalSize = $GLOBALS['config']['MAX_TOTAL_ATTACHMENT_SIZE_PER_TICKET'] ?? 104857600; +if ($attachmentModel->getTotalSizeForTicket($ticketId) + $file['size'] > $maxTotalSize) { + ResponseHelper::error( + 'This upload would exceed the ticket\'s total attachment size limit of ' + . AttachmentModel::formatFileSize($maxTotalSize) + ); +} + // Get MIME type $finfo = new finfo(FILEINFO_MIME_TYPE); $mimeType = $finfo->file($file['tmp_name']); @@ -265,7 +282,6 @@ if (empty($originalFilename)) { // Save to database try { - $attachmentModel = new AttachmentModel($conn); $attachmentId = $attachmentModel->addAttachment( $ticketId, $uniqueFilename, diff --git a/config/config.php b/config/config.php index 3ffb242..80ab44e 100644 --- a/config/config.php +++ b/config/config.php @@ -115,6 +115,8 @@ $GLOBALS['config'] = [ // File upload settings 'MAX_UPLOAD_SIZE' => 10485760, // 10MB in bytes + 'MAX_ATTACHMENTS_PER_TICKET' => 50, + 'MAX_TOTAL_ATTACHMENT_SIZE_PER_TICKET' => 104857600, // 100MB in bytes 'ALLOWED_FILE_TYPES' => [ 'image/jpeg', 'image/png', From 818af137f300efd3a383119c4bac12508be56752 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 14:18:54 -0400 Subject: [PATCH 28/29] Add touch-event fallback to lt.sortable for kanban drag-and-drop (#65) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lt.sortable only wired dragstart/dragend/dragover/drop — the native HTML5 Drag-and-Drop API. iOS Safari doesn't implement HTML5 DnD on arbitrary elements at all, and mobile Chrome's support is poor, so kanban card status-drag was effectively unusable via touch, despite README's "Touch-friendly controls" claim. Added touchstart/touchmove/touchend/touchcancel handling that mirrors the existing mouse-based behavior: a small movement threshold (8px) distinguishes a tap/scroll from drag intent, the dragged card is repositioned via fixed positioning to follow the finger (reparented to document.body to avoid clipping by an overflow:hidden ancestor), and elementFromPoint resolves the hover target for the same placeholder-insertion logic dragover already uses, including cross-column moves via the shared group check. touchmove/touchend/touchcancel are registered on document rather than the sortable list itself: since touch events keep targeting their touchstart element for the whole gesture regardless of DOM mutations, and the dragged item gets reparented to document.body mid-drag, a listener on the original list would stop receiving bubbled events once that reparenting happens. lt.sortable lives in base.js, the shared web_template copy used by other LotusGuild apps (per its own header comment) — this fix should be contributed upstream too, not just kept local to this repo. Verified with a jsdom harness (stubbing getBoundingClientRect and elementFromPoint against known layouts) covering: sub-threshold movement not starting a drag, same-column reorder, cross-column move with correct final DOM parent and order, and touchcancel cleanly resetting state. This caught a real bug during development — an earlier version listened on the list element for touchmove/touchend, which silently stopped receiving events after the drag-start reparenting. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/base.js | 95 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/assets/js/base.js b/assets/js/base.js index 8925325..3547921 100644 --- a/assets/js/base.js +++ b/assets/js/base.js @@ -2475,6 +2475,101 @@ list.addEventListener('drop', e => { e.preventDefault(); }); + // Touch fallback — iOS Safari doesn't implement HTML5 drag-and-drop on + // arbitrary elements at all, and mobile Chrome's support is poor, so + // kanban drag was effectively unusable via touch without this. Touch + // events for a given touch point are always dispatched to the element + // touchstart fired on (per spec), so per-list local state here is safe; + // cross-list moves are resolved via elementFromPoint against the live + // finger position, same as dragover does via e.target above. + const DRAG_THRESHOLD = 8; // px of movement before a touch starts a drag + let _touchItem = null, _touchDragging = false; + let _touchStartX = 0, _touchStartY = 0, _touchOffsetX = 0, _touchOffsetY = 0; + + function _touchTargetList(x, y) { + const el = document.elementFromPoint(x, y); + const found = el ? el.closest('[data-sortable-group]') : null; + return found && (found === list || _sameGroup(found)) ? found : null; + } + + list.addEventListener('touchstart', e => { + const item = e.target.closest('[data-sortable-item]'); + if (!item || !list.contains(item)) return; + if (handle && !e.target.closest(handle)) return; + const t = e.touches[0]; + _touchItem = item; + _touchDragging = false; + _touchStartX = t.clientX; + _touchStartY = t.clientY; + }, { passive: true }); + + // touchmove/touchend/touchcancel are registered on document, not list: + // once the dragged item is reparented to document.body below, it's no + // longer a descendant of list, so events targeting it (touch events + // keep targeting their touchstart element for the whole gesture) would + // stop bubbling to a listener on list. + document.addEventListener('touchmove', e => { + if (!_touchItem) return; + const t = e.touches[0]; + + if (!_touchDragging) { + if (Math.abs(t.clientX - _touchStartX) < DRAG_THRESHOLD && Math.abs(t.clientY - _touchStartY) < DRAG_THRESHOLD) return; + // Drag intent confirmed — take over from here, blocking page scroll. + _touchDragging = true; + _srtDragging = _touchItem; + _srtSrcList = list; + _srtPlaceholder = _makePlaceholder(_touchItem); + _touchItem.classList.add('is-dragging'); + const rect = _touchItem.getBoundingClientRect(); + _touchOffsetX = _touchStartX - rect.left; + _touchOffsetY = _touchStartY - rect.top; + _touchItem.parentNode.insertBefore(_srtPlaceholder, _touchItem); + _touchItem.style.position = 'fixed'; + _touchItem.style.zIndex = '1000'; + _touchItem.style.width = rect.width + 'px'; + _touchItem.style.pointerEvents = 'none'; + document.body.appendChild(_touchItem); // avoid clipping by an overflow:hidden ancestor + } + + e.preventDefault(); + _touchItem.style.left = (t.clientX - _touchOffsetX) + 'px'; + _touchItem.style.top = (t.clientY - _touchOffsetY) + 'px'; + + const targetList = _touchTargetList(t.clientX, t.clientY); + if (!targetList) return; + const overEl = document.elementFromPoint(t.clientX, t.clientY); + const over = overEl ? overEl.closest('[data-sortable-item]') : null; + if (over && over !== _srtDragging && targetList.contains(over)) { + const rect = over.getBoundingClientRect(); + targetList.insertBefore(_srtPlaceholder, t.clientY < rect.top + rect.height / 2 ? over : over.nextSibling); + } else if (!targetList.contains(_srtPlaceholder)) { + targetList.appendChild(_srtPlaceholder); + } + }, { passive: false }); + + function _touchEnd() { + if (_touchDragging && _srtDragging) { + _srtDragging.classList.remove('is-dragging'); + _srtDragging.style.position = ''; + _srtDragging.style.zIndex = ''; + _srtDragging.style.width = ''; + _srtDragging.style.pointerEvents = ''; + _srtDragging.style.left = ''; + _srtDragging.style.top = ''; + if (_srtPlaceholder && _srtPlaceholder.parentNode) { + _srtPlaceholder.parentNode.insertBefore(_srtDragging, _srtPlaceholder); + _srtPlaceholder.remove(); + } + if (onSort) onSort(_getItems(), _srtDragging); + bus.emit('sortable:change', { list, items: _getItems(), moved: _srtDragging }); + } + _touchItem = null; _touchDragging = false; + _srtDragging = null; _srtPlaceholder = null; _srtSrcList = null; + } + + document.addEventListener('touchend', _touchEnd); + document.addEventListener('touchcancel', _touchEnd); + return { refresh() { Array.from(list.children).forEach(child => { if (!child.hasAttribute('data-sortable-item')) _mark(child); }); }, getOrder: () => _getItems().map(el => el.dataset.id || el.textContent.trim()), From 3cca956ee7443a4a40268296ee609319edb4a789 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 8 Sep 2026 14:21:39 -0400 Subject: [PATCH 29/29] Preserve native undo/redo in markdown toolbar buttons (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit insertMarkdownFormat, insertMarkdownText, toolbarList, toolbarHeading, and toolbarQuote all set textarea.value = ... directly. Assigning .value programmatically discards the browser's entire native undo stack (vs. document.execCommand('insertText', ...), which preserves it) — e.g. type a paragraph, click Bold, then Ctrl+Z undid the whole paragraph instead of just the bold markup. Added insertTextPreservingUndo(), which selects the exact range being replaced and routes through execCommand('insertText', ...) — the same mechanism real typing uses — falling back to the old direct assignment (losing undo, matching prior behavior) only if execCommand is unavailable or unsuccessful. Verified with a jsdom harness that the fallback path (jsdom doesn't implement execCommand, since native undo is a real-browser-only feature untestable via jsdom) produces byte-identical resulting text and cursor positions to the original implementation across all 5 toolbar functions, for both selected and cursor-only cases. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MGDKHiU5RJdo3dqQUDow3X --- assets/js/markdown.js | 47 +++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/assets/js/markdown.js b/assets/js/markdown.js index 7bba4ee..b9653ce 100644 --- a/assets/js/markdown.js +++ b/assets/js/markdown.js @@ -354,6 +354,33 @@ window.renderMarkdownElements = renderMarkdownElements; // Rich Text Editor Toolbar Functions // ======================================== +/** + * Replace textarea.value.substring(selStart, selEnd) with replacementText, + * preserving the browser's native undo/redo stack via + * document.execCommand('insertText', ...) -- the same mechanism real typing + * uses -- instead of a direct .value assignment, which discards the entire + * undo history. Falls back to a direct assignment (losing undo, matching the + * old behavior) only if execCommand is unavailable or unsuccessful. + */ +function insertTextPreservingUndo(textarea, replacementText, selStart, selEnd) { + textarea.focus(); + textarea.setSelectionRange(selStart, selEnd); + + let inserted = false; + if (typeof document.execCommand === 'function') { + try { + inserted = document.execCommand('insertText', false, replacementText); + } catch (e) { + inserted = false; + } + } + + if (!inserted) { + const text = textarea.value; + textarea.value = text.substring(0, selStart) + replacementText + text.substring(selEnd); + } +} + /** * Insert markdown formatting around selection */ @@ -363,16 +390,13 @@ function insertMarkdownFormat(textareaId, prefix, suffix) { const start = textarea.selectionStart; const end = textarea.selectionEnd; - const text = textarea.value; - const selectedText = text.substring(start, end); + const selectedText = textarea.value.substring(start, end); - // Insert formatting - const newText = text.substring(0, start) + prefix + selectedText + suffix + text.substring(end); - textarea.value = newText; + insertTextPreservingUndo(textarea, prefix + selectedText + suffix, start, end); // Set cursor position if (selectedText) { - textarea.setSelectionRange(start + prefix.length, end + prefix.length); + textarea.setSelectionRange(start + prefix.length, start + prefix.length + selectedText.length); } else { textarea.setSelectionRange(start + prefix.length, start + prefix.length); } @@ -391,9 +415,10 @@ function insertMarkdownText(textareaId, text) { if (!textarea) return; const start = textarea.selectionStart; - const value = textarea.value; - textarea.value = value.substring(0, start) + text + value.substring(start); + // Matches the prior behavior: insert before the selection start without + // deleting any currently-selected text (a collapsed replace range). + insertTextPreservingUndo(textarea, text, start, start); textarea.setSelectionRange(start + text.length, start + text.length); textarea.focus(); @@ -453,7 +478,7 @@ function toolbarList(textareaId) { } // Insert list marker at beginning of line - textarea.value = text.substring(0, lineStart) + '- ' + text.substring(lineStart); + insertTextPreservingUndo(textarea, '- ', lineStart, lineStart); textarea.setSelectionRange(start + 2, start + 2); textarea.focus(); @@ -474,7 +499,7 @@ function toolbarHeading(textareaId) { } // Insert heading marker at beginning of line - textarea.value = text.substring(0, lineStart) + '## ' + text.substring(lineStart); + insertTextPreservingUndo(textarea, '## ', lineStart, lineStart); textarea.setSelectionRange(start + 3, start + 3); textarea.focus(); @@ -495,7 +520,7 @@ function toolbarQuote(textareaId) { } // Insert quote marker at beginning of line - textarea.value = text.substring(0, lineStart) + '> ' + text.substring(lineStart); + insertTextPreservingUndo(textarea, '> ', lineStart, lineStart); textarea.setSelectionRange(start + 2, start + 2); textarea.focus();