From 9e83f8903a02a368770fa8a8d012e6f44a2709f1 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 13:30:59 -0400 Subject: [PATCH 1/5] Prune watchers when a ticket's visibility is tightened (#73) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TicketModel::updateVisibility() only updated the tickets row — it never touched ticket_watchers. A user watching a public ticket that's later made confidential/internal, and who isn't creator/assignee/ admin/in the new visibility_groups, kept receiving Matrix notifications (title + redacted activity preview) about a ticket canUserAccessTicket() would now reject them from opening directly. After a successful visibility update, re-evaluates every current watcher against the new visibility rules via the same canUserAccessTicket() check the rest of the app uses, and removes any who no longer qualify. Verified against real MariaDB: tightening to confidential correctly drops watchers with no standing access while keeping an admin watcher; tightening to internal with a specific group correctly keeps a watcher in that group and drops one who isn't. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- models/TicketModel.php | 59 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/models/TicketModel.php b/models/TicketModel.php index 7e05e42..5079224 100644 --- a/models/TicketModel.php +++ b/models/TicketModel.php @@ -773,9 +773,68 @@ class TicketModel $stmt->bind_param("ssis", $visibility, $visibilityGroups, $updatedBy, $ticketId); $result = $stmt->execute(); $stmt->close(); + + if ($result) { + $this->pruneWatchersForVisibility($ticketId, $visibility, $visibilityGroups); + } + return $result; } + /** + * Remove any watchers who no longer qualify for a ticket's access rules + * after its visibility was tightened. Without this, a user watching a + * ticket that's later made confidential/internal (and who isn't + * creator/assignee/admin/in the new visibility_groups) keeps receiving + * Matrix notifications about a ticket canUserAccessTicket() would now + * reject them from opening directly. + */ + private function pruneWatchersForVisibility(string $ticketId, string $visibility, ?string $visibilityGroups): void + { + $ticket = $this->getTicketById($ticketId); + if (!$ticket) { + return; + } + // getTicketById() reflects the just-committed UPDATE, but set these + // explicitly so pruning is correct even if a caller reorders things. + $ticket['visibility'] = $visibility; + $ticket['visibility_groups'] = $visibilityGroups; + + $sql = "SELECT tw.user_id, u.is_admin, u.`groups` + FROM ticket_watchers tw + JOIN users u ON tw.user_id = u.user_id + WHERE tw.ticket_id = ?"; + $stmt = $this->conn->prepare($sql); + $stmt->bind_param('s', $ticketId); + $stmt->execute(); + $watchers = $stmt->get_result()->fetch_all(MYSQLI_ASSOC); + $stmt->close(); + + $toRemove = []; + foreach ($watchers as $watcher) { + $watcherUser = [ + 'user_id' => $watcher['user_id'], + 'is_admin' => $watcher['is_admin'], + 'groups' => $watcher['groups'], + ]; + if (!$this->canUserAccessTicket($ticket, $watcherUser)) { + $toRemove[] = $watcher['user_id']; + } + } + + if (empty($toRemove)) { + return; + } + + $placeholders = implode(',', array_fill(0, count($toRemove), '?')); + $delSql = "DELETE FROM ticket_watchers WHERE ticket_id = ? AND user_id IN ($placeholders)"; + $delStmt = $this->conn->prepare($delSql); + $types = 's' . str_repeat('i', count($toRemove)); + $delStmt->bind_param($types, $ticketId, ...$toRemove); + $delStmt->execute(); + $delStmt->close(); + } + /** * Delete a ticket and all its associated records. * Admin-only operation. Removes comments, attachments, watchers, dependencies. From 3db3749c466b17b8b64c9138ab7f6dc523f4fdb8 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 13:31:08 -0400 Subject: [PATCH 2/5] Re-check ticket visibility before surfacing in-app notifications (#48) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four notification queries in api/notifications.php (assign, comment, status-change, mention) were scoped purely by created_by/assigned_to/ticket_watchers membership and historical audit_log contents — never by canUserAccessTicket(). If a ticket's visibility was later tightened, or a user's group/watcher access revoked, a notification still surfaced in their bell dropdown, disclosing the ticket's title and that activity occurred even though opening the ticket itself would now be blocked. Batch-fetches the tickets referenced by all candidate notifications (via the existing getTicketsByIds()) and filters out any whose current state canUserAccessTicket() would reject for the requesting user, before formatting the response — so a notification for a ticket the user can no longer see simply disappears rather than lingering as a disclosure. Verified against real MariaDB with a running server: an assignment notification is visible while the user is the assignee of a public ticket, and disappears once the ticket is reassigned away and made confidential. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- api/notifications.php | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/api/notifications.php b/api/notifications.php index ff22d70..b3d5dbd 100644 --- a/api/notifications.php +++ b/api/notifications.php @@ -15,8 +15,10 @@ require_once __DIR__ . '/bootstrap.php'; require_once dirname(__DIR__) . '/models/UserPreferencesModel.php'; +require_once dirname(__DIR__) . '/models/TicketModel.php'; $prefsModel = new UserPreferencesModel($conn); +$ticketModel = new TicketModel($conn); // ── POST: mark all read (update last_seen timestamp) ────────────── if ($_SERVER['REQUEST_METHOD'] === 'POST') { @@ -204,7 +206,44 @@ foreach (array_merge($assignRows, $commentRows, $statusRows, $mentionRows) as $r $all[] = $row; } usort($all, fn($a, $b) => strcmp($b['created_at'], $a['created_at'])); -$all = array_slice($all, 0, 30); + +// Re-check current ticket visibility before surfacing anything: a +// notification's audit_log entry reflects historical activity, but the +// ticket's visibility (or the user's group/watcher standing) may have +// tightened since. Without this, a notification still discloses the +// ticket's title and that activity occurred to someone who currently +// shouldn't see it, even though the ticket view's own access check would +// correctly reject them from opening it. +$candidateTicketIds = []; +foreach ($all as $row) { + $details = json_decode($row['details'] ?? '{}', true) ?? []; + $actionType = ($row['action_type'] === 'create' && $row['entity_type'] === 'comment') + ? 'comment' + : $row['action_type']; + $tid = ($actionType === 'comment' || $actionType === 'mention') + ? ($details['ticket_id'] ?? 0) + : $row['entity_id']; + if ($tid) { + $candidateTicketIds[(string)$tid] = true; + } +} +$ticketsById = !empty($candidateTicketIds) + ? $ticketModel->getTicketsByIds(array_keys($candidateTicketIds)) + : []; + +$all = array_filter($all, function ($row) use ($ticketsById, $currentUser, $ticketModel) { + $details = json_decode($row['details'] ?? '{}', true) ?? []; + $actionType = ($row['action_type'] === 'create' && $row['entity_type'] === 'comment') + ? 'comment' + : $row['action_type']; + $tid = (string)(($actionType === 'comment' || $actionType === 'mention') + ? ($details['ticket_id'] ?? 0) + : $row['entity_id']); + $ticket = $ticketsById[$tid] ?? null; + return $ticket && $ticketModel->canUserAccessTicket($ticket, $currentUser); +}); + +$all = array_slice(array_values($all), 0, 30); // Format for response $notifications = []; From 5b96e75ff68b0744f2bfe6ca1480acd60a42f907 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 13:31:15 -0400 Subject: [PATCH 3/5] Use lt.api instead of raw fetch() in notification bell (#57) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit layout_footer.php's loadNotifications() and "mark all read" handler called fetch() directly instead of lt.api.*, violating the project's own documented convention (README Dev Notes #20). api/bootstrap.php rotates the CSRF token on every successful write and returns it in the response's csrf_token field; lt.api.* reads that and updates window.CSRF_TOKEN, but a raw fetch() never does — so after "mark all read", the server had rotated its token but the client's cached one was stale, causing the user's next write anywhere else in the app to fail once with "Invalid CSRF token" before self-healing. Replaced both fetch() calls with lt.api.get/post, which also drops the now-redundant manual header/credentials boilerplate. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- views/layout_footer.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/views/layout_footer.php b/views/layout_footer.php index 144b561..8992dd6 100644 --- a/views/layout_footer.php +++ b/views/layout_footer.php @@ -235,8 +235,7 @@ } function loadNotifications() { - return fetch('/api/notifications.php', { credentials: 'same-origin' }) - .then(function(r) { return r.json(); }) + return lt.api.get('/api/notifications.php') .then(function(data) { renderNotifications(data); return true; }) .catch(function() { list.innerHTML = '
Could not load
'; @@ -251,11 +250,7 @@ if (clearBtn) { clearBtn.addEventListener('click', function() { - fetch('/api/notifications.php', { - method: 'POST', credentials: 'same-origin', - headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': window.CSRF_TOKEN || '' }, - body: JSON.stringify({ action: 'mark_read' }) - }).then(loadNotifications); + lt.api.post('/api/notifications.php', { action: 'mark_read' }).then(loadNotifications); }); } From ae12fcd6fd6b551e5d87dc9046d1a22a2060c4d7 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 13:31:25 -0400 Subject: [PATCH 4/5] Auto-retry once after CSRF token resync in lt.api (#86) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lt.api's fetch wrapper (_apiFetchAuth in base.js — the live implementation lt.api.* resolves to) already resynced window.CSRF_TOKEN from a 403 response's csrf_token field, but still threw immediately — every caller saw a raw "Invalid CSRF token" error on the FIRST attempt, with no transparent retry. Since CsrfMiddleware's token lifetime (1h) is shorter than the session idle timeout (5h), this was a routine, fully recoverable case (an hour of page inactivity, or a write in another tab rotating the shared token), not a real rejection. After resyncing the token from a 403 body that carries one, now retries the original request exactly once with the fresh token before surfacing an error — transparent to the caller on the common case, with a `retried` flag preventing more than one retry so a genuinely broken session still fails cleanly instead of looping. Verified via jsdom with a mocked fetch: a 403-then-succeeds sequence resolves successfully with exactly 2 network calls and the correct final token; a persistently-403 sequence still throws after exactly 2 calls (no infinite retry). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- assets/js/base.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/assets/js/base.js b/assets/js/base.js index 3547921..397c58f 100644 --- a/assets/js/base.js +++ b/assets/js/base.js @@ -2801,7 +2801,7 @@ }; // Patch lt.api — auth-aware wrapper (renamed to avoid strict-mode duplicate declaration) - async function _apiFetchAuth(method, url, body) { + async function _apiFetchAuth(method, url, body, retried) { if (_authAccess && auth.isExpiringSoon()) await auth.refresh(); const opts = { method, headers: Object.assign({ 'Content-Type': 'application/json' }, csrfHeaders()) }; if (_authAccess) opts.headers['Authorization'] = 'Bearer ' + _authAccess; @@ -2821,6 +2821,15 @@ // Resync CSRF token from any response body that carries a fresh one // (bootstrap rotates on success and returns the current token on rejection). if (data && data.csrf_token) global.CSRF_TOKEN = data.csrf_token; + // Auto-retry once on a stale-CSRF-token 403: the token lifetime (1h) is + // shorter than the session idle timeout (5h), so this is a routine, + // recoverable case (an hour of inactivity, or a write in another tab + // rotating the shared token) rather than a real rejection — resyncing + // above already has the fresh token, so silently resending once succeeds + // transparently instead of surfacing a confusing error on the first try. + if (resp.status === 403 && !retried && data && data.csrf_token) { + return _apiFetchAuth(method, url, body, true); + } if (!resp.ok) { const err = new Error(data.error || data.message || 'HTTP ' + resp.status); err.data = data; From fca0b42726e937f64a7a2b809591b33d7e3ebdbb Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 13:31:33 -0400 Subject: [PATCH 5/5] Validate field_type against the allowed enum in custom field definitions (#50) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setValue()/is_required/select-options half of this issue was already fixed incidentally by #47's new api/ticket_custom_fields.php endpoint. The remaining gap: createDefinition()/updateDefinition() never validated field_type against the six values the schema's enum() actually allows (text/textarea/select/checkbox/date/number), so a malformed type could be stored via the admin API and break whatever UI renders it later. Added an ALLOWED_FIELD_TYPES allowlist check at the top of both methods, returning the same ['success' => false, 'error' => ...] shape they already use for a DB failure — api/custom_fields.php already propagates that shape correctly with no changes needed there. Verified against real MariaDB: an invalid field_type is rejected on both create and update, while a valid one still succeeds. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- models/CustomFieldModel.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/models/CustomFieldModel.php b/models/CustomFieldModel.php index 4ebb3c0..cc80aa2 100644 --- a/models/CustomFieldModel.php +++ b/models/CustomFieldModel.php @@ -8,6 +8,9 @@ class CustomFieldModel { private $conn; + // Must match custom_field_definitions.field_type's enum() in the schema. + private const ALLOWED_FIELD_TYPES = ['text', 'textarea', 'select', 'checkbox', 'date', 'number']; + public function __construct($conn) { $this->conn = $conn; @@ -87,6 +90,10 @@ class CustomFieldModel */ public function createDefinition($data) { + if (!in_array($data['field_type'] ?? '', self::ALLOWED_FIELD_TYPES, true)) { + return ['success' => false, 'error' => 'Invalid field_type']; + } + $options = null; if (isset($data['field_options']) && !empty($data['field_options'])) { $options = json_encode($data['field_options']); @@ -129,6 +136,10 @@ class CustomFieldModel */ public function updateDefinition($fieldId, $data) { + if (!in_array($data['field_type'] ?? '', self::ALLOWED_FIELD_TYPES, true)) { + return ['success' => false, 'error' => 'Invalid field_type']; + } + $options = null; if (isset($data['field_options']) && !empty($data['field_options'])) { $options = json_encode($data['field_options']);