Address remaining review items: Synapse caching, cycle detection, cache/ratelimit/kanban

- SynapseHelper: memoize username->Matrix-ID lookups per-request (incl. negative
  results) and add an overall time budget to resolveUsernames() plus a 2s connect
  timeout, so notifying N watchers with a slow/unreachable Synapse can't stall the
  request for N x 5s. (Chosen over async/queue per maintainer.)
- DependencyModel: fix cycle detection treating 'blocks' and 'blocked_by' as the
  same edge direction. They are inverse relationships (single row each, no mirror
  row), so the traversal now walks a unified precedence graph (blocks: ticket->
  depends_on; blocked_by: depends_on->ticket) and wouldCreateCycle normalizes the
  new edge's direction. Prevents both false-positive and missed cycles.
- CacheHelper: anchor prefix-delete to exact key boundaries (bare prefix or
  prefix + '_' + md5) so delete('workflow') can't wipe a 'workflow_rules' cache.
- RateLimitMiddleware: hold an exclusive flock across the per-IP counter's
  read-modify-write so concurrent requests can't both read N and write N+1
  (undercounting past the limit). Fails open if the file can't be locked.
- dashboard.js: kanban status update now uses lt.api.post (per no-raw-fetch
  convention) and reverts the card AND the optimistic column counts on failure
  (the old raw-fetch catch left the card moved without reverting).
- BulkOperationsModel: document that bulk_status/bulk_close intentionally bypass
  workflow transition validation (admin override, by design).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

api/check_duplicates.php

@@ -50,12 +50,24 @@ $sql = "SELECT ticket_id, title, status, priority, created_at
 
 $types = "ss" . $visFilter['types'];
 $params = array_merge([$searchTerm, $soundexTitle], $visFilter['params']);
-$stmt = $conn->prepare($sql);
-if (!empty($params)) {
-    $stmt->bind_param($types, ...$params);
+
+// Duplicate detection is advisory (it must not block ticket creation), so on any
+// DB error degrade gracefully to "no duplicates" rather than fataling the request.
+// mysqli may throw (default exception mode) or return false depending on config.
+try {
+    $stmt = $conn->prepare($sql);
+    if (!$stmt) {
+        throw new RuntimeException('prepare failed: ' . $conn->error);
+    }
+    if (!empty($params)) {
+        $stmt->bind_param($types, ...$params);
+    }
+    $stmt->execute();
+    $result = $stmt->get_result();
+} catch (Throwable $e) {
+    error_log('check_duplicates: ' . $e->getMessage());
+    ResponseHelper::success(['duplicates' => []]);
 }
-$stmt->execute();
-$result = $stmt->get_result();
 
 while ($row = $result->fetch_assoc()) {
     // Calculate similarity score

api/manage_workflows.php

@@ -82,6 +82,15 @@ try {
         case 'POST':
             $data = json_decode(file_get_contents('php://input'), true);
 
+            $wfValid = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed'];
+            if (
+                !in_array($data['from_status'] ?? '', $wfValid, true)
+                || !in_array($data['to_status'] ?? '', $wfValid, true)
+            ) {
+                http_response_code(400);
+                echo json_encode(['success' => false, 'error' => 'from_status and to_status must be valid ticket statuses']);
+                exit;
+            }
             if (($data['from_status'] ?? '') === ($data['to_status'] ?? '')) {
                 http_response_code(400);
                 echo json_encode(['success' => false, 'error' => 'From Status and To Status cannot be the same']);
@@ -125,6 +134,15 @@ try {
 
             $data = json_decode(file_get_contents('php://input'), true);
 
+            $wfValid = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed'];
+            if (
+                !in_array($data['from_status'] ?? '', $wfValid, true)
+                || !in_array($data['to_status'] ?? '', $wfValid, true)
+            ) {
+                http_response_code(400);
+                echo json_encode(['success' => false, 'error' => 'from_status and to_status must be valid ticket statuses']);
+                exit;
+            }
             if (($data['from_status'] ?? '') === ($data['to_status'] ?? '')) {
                 http_response_code(400);
                 echo json_encode(['success' => false, 'error' => 'From Status and To Status cannot be the same']);

assets/js/dashboard.js

@@ -1219,29 +1219,30 @@ function populateKanbanCards() {
             if (dec) dec.textContent = '(' + Math.max(0, (parseInt(dec.textContent.replace(/\D/g,''),10)||1) - 1) + ')';
             if (inc) inc.textContent = '(' + ((parseInt(inc.textContent.replace(/\D/g,''),10)||0) + 1) + ')';
 
-            // POST status update
-            fetch('/api/update_ticket.php', {
-                method: 'POST',
-                credentials: 'same-origin',
-                headers: { 'Content-Type': 'application/json', 'X-CSRF-Token': window.CSRF_TOKEN || '' },
-                body: JSON.stringify({ ticket_id: String(ticketId), status: newStatus })
-            })
-            .then(r => r.json())
-            .then(data => {
-                if (data.success) {
-                    lt.toast.success('Ticket #' + ticketId + ' → ' + newStatus, 2500);
-                    movedCard.dataset.status = newStatus;
-                } else {
-                    lt.toast.error('Status update failed: ' + (data.error || 'Unknown error'));
-                    // Revert: put card back in original column
-                    const origCol = document.getElementById(Object.keys(colStatusMap).find(k => colStatusMap[k] === oldStatus));
-                    if (origCol) origCol.appendChild(movedCard);
-                    movedCard.dataset.status = oldStatus;
-                }
-            })
-            .catch(() => {
-                lt.toast.error('Network error — status not saved');
-            });
+            // Revert the card to its original column and undo the optimistic counts.
+            const revert = function () {
+                const origCol = document.getElementById(Object.keys(colStatusMap).find(k => colStatusMap[k] === oldStatus));
+                if (origCol) origCol.appendChild(movedCard);
+                movedCard.dataset.status = oldStatus;
+                if (dec) dec.textContent = '(' + ((parseInt(dec.textContent.replace(/\D/g, ''), 10) || 0) + 1) + ')';
+                if (inc) inc.textContent = '(' + Math.max(0, (parseInt(inc.textContent.replace(/\D/g, ''), 10) || 1) - 1) + ')';
+            };
+
+            // POST status update via the shared wrapper (adds CSRF + JSON, throws on non-2xx)
+            lt.api.post('/api/update_ticket.php', { ticket_id: String(ticketId), status: newStatus })
+                .then(function (data) {
+                    if (data && data.success) {
+                        lt.toast.success('Ticket #' + ticketId + ' → ' + newStatus, 2500);
+                        movedCard.dataset.status = newStatus;
+                    } else {
+                        lt.toast.error('Status update failed: ' + ((data && data.error) || 'Unknown error'));
+                        revert();
+                    }
+                })
+                .catch(function () {
+                    lt.toast.error('Status update failed — reverting');
+                    revert();
+                });
         }
 
         Object.keys(columns).forEach(status => {

create_ticket_api.php

@@ -231,6 +231,22 @@ $priority    = $data['priority'] ?? '4';
 $category    = (string)($data['category']    ?? 'General');
 $type        = (string)($data['type']        ?? 'Issue');
 
+// Validate externally-supplied status and priority. (category/type are free-form
+// in this schema.) A non-numeric priority would otherwise cast to 0 and escalate
+// the ticket below P1 on the dedup/update path.
+$validStatuses = $GLOBALS['config']['TICKET_STATUSES'] ?? ['Open', 'Pending', 'In Progress', 'Closed'];
+if (!in_array($status, $validStatuses, true)) {
+    http_response_code(400);
+    echo json_encode(['success' => false, 'error' => 'Invalid status']);
+    exit;
+}
+if (!is_numeric($priority) || (int)$priority < 1 || (int)$priority > 5) {
+    http_response_code(400);
+    echo json_encode(['success' => false, 'error' => 'Invalid priority (must be 1-5)']);
+    exit;
+}
+$priority = (int)$priority;
+
 $ticketHash = generateTicketHash($data);
 $auditLog   = new AuditLogModel($conn);
 

helpers/CacheHelper.php

@@ -125,16 +125,23 @@ class CacheHelper
             return !file_exists($filePath) || @unlink($filePath);
         }
 
-        // Delete all files with this prefix
-        $pattern = self::getCacheDir() . '/' . preg_replace('/[^a-zA-Z0-9_]/', '_', $prefix) . '*.json';
-        $files = glob($pattern);
+        // Delete all entries for this prefix. A key is either the bare prefix or
+        // prefix + '_' + md5(identifier) (32 hex chars, see makeKey). Match exactly
+        // that so a prefix can't clobber a different prefix that merely shares a
+        // leading substring — e.g. delete('workflow') must not wipe 'workflow_rules'.
+        $safePrefix = preg_replace('/[^a-zA-Z0-9_]/', '_', $prefix);
+        $keyRegex   = '/^' . preg_quote($safePrefix, '/') . '(_[0-9a-f]{32})?$/';
+
+        $files = glob(self::getCacheDir() . '/' . $safePrefix . '*.json') ?: [];
         foreach ($files as $file) {
-            @unlink($file);
+            if (preg_match($keyRegex, basename($file, '.json'))) {
+                @unlink($file);
+            }
         }
 
-        // Clear memory cache entries with this prefix
+        // Clear matching memory cache entries
         foreach (array_keys(self::$memoryCache) as $key) {
-            if (strpos($key, $prefix) === 0) {
+            if (preg_match($keyRegex, $key)) {
                 unset(self::$memoryCache[$key]);
             }
         }

helpers/NotificationHelper.php

@@ -164,32 +164,38 @@ class NotificationHelper
             return;
         }
 
-        // Fetch watcher usernames, excluding the actor so they don't notify themselves
-        if ($excludeUserId !== null) {
-            $sql  = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ? AND tw.user_id != ?";
-            $stmt = $conn->prepare($sql);
-        } else {
-            $sql  = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ?";
-            $stmt = $conn->prepare($sql);
-        }
-        // Notifications are best-effort; if the watchers table is absent or the
-        // statement fails to prepare, skip silently rather than fataling the
-        // request that already committed its DB change.
-        if (!$stmt) {
-            return;
-        }
-        if ($excludeUserId !== null) {
-            $stmt->bind_param("ii", $ticketId, $excludeUserId);
-        } else {
-            $stmt->bind_param("i", $ticketId);
-        }
-        $stmt->execute();
-        $result = $stmt->get_result();
-        $stmt->close();
-
+        // Fetch watcher usernames, excluding the actor so they don't notify
+        // themselves. Notifications are best-effort: if the watchers table is
+        // absent or the query fails, skip silently rather than fataling the
+        // request that already committed its DB change. mysqli may either throw
+        // (default exception mode) or return false, so handle both.
         $usernames = [];
-        while ($row = $result->fetch_assoc()) {
-            $usernames[] = $row['username'];
+        try {
+            if ($excludeUserId !== null) {
+                $sql  = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ? AND tw.user_id != ?";
+                $stmt = $conn->prepare($sql);
+            } else {
+                $sql  = "SELECT u.username FROM ticket_watchers tw JOIN users u ON tw.user_id = u.user_id WHERE tw.ticket_id = ?";
+                $stmt = $conn->prepare($sql);
+            }
+            if (!$stmt) {
+                return;
+            }
+            if ($excludeUserId !== null) {
+                $stmt->bind_param("ii", $ticketId, $excludeUserId);
+            } else {
+                $stmt->bind_param("i", $ticketId);
+            }
+            $stmt->execute();
+            $result = $stmt->get_result();
+            $stmt->close();
+
+            while ($row = $result->fetch_assoc()) {
+                $usernames[] = $row['username'];
+            }
+        } catch (\Throwable $e) {
+            error_log('NotificationHelper::notifyWatchers watcher lookup failed: ' . $e->getMessage());
+            return;
         }
 
         if (empty($usernames)) {

helpers/SynapseHelper.php

@@ -4,8 +4,9 @@
  * SynapseHelper
  *
  * Resolves local (SSO) usernames → Matrix user IDs by querying the
- * Synapse Admin REST API directly.  No caching — every call is live
- * so results never go stale.
+ * Synapse Admin REST API directly. Results are memoized per-request (not
+ * across requests, so they don't go stale between requests), and a batch
+ * resolve has an overall time budget to bound request latency.
  *
  * Required config (.env) keys:
  *   MATRIX_DOMAIN      e.g. matrix.lotusguild.org
@@ -14,6 +15,12 @@
  */
 class SynapseHelper
 {
+    /** Per-request memo of username => Matrix ID|null, so repeat watchers are free. */
+    private static array $cache = [];
+
+    /** Total wall-clock budget (seconds) for a single resolveUsernames() batch. */
+    private const RESOLVE_BUDGET_SECONDS = 5;
+
     /**
      * Resolve a local SSO username to its Matrix user ID.
      *
@@ -29,6 +36,11 @@ class SynapseHelper
      */
     public static function resolveUsername(string $username): ?string
     {
+        // Serve from the per-request cache when we've already looked this up.
+        if (array_key_exists($username, self::$cache)) {
+            return self::$cache[$username];
+        }
+
         $baseUrl = $GLOBALS['config']['SYNAPSE_ADMIN_URL']  ?? null;
         $token   = $GLOBALS['config']['SYNAPSE_ADMIN_TOKEN'] ?? null;
         $domain  = $GLOBALS['config']['MATRIX_DOMAIN']      ?? null;
@@ -49,6 +61,7 @@ class SynapseHelper
             'Accept: application/json',
         ]);
         curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
+        curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 2); // fail fast when Synapse is unreachable
         curl_setopt($ch, CURLOPT_TIMEOUT, 5);
 
         $body      = curl_exec($ch);
@@ -56,25 +69,24 @@ class SynapseHelper
         $curlError = curl_error($ch);
         curl_close($ch);
 
+        $resolved = null;
         if ($curlError) {
             error_log("SynapseHelper: cURL error resolving '{$username}': {$curlError}");
-            return null;
-        }
-
-        if ($httpCode === 200) {
+        } elseif ($httpCode === 200) {
             $data = json_decode($body, true);
             // Confirm the response contains the name we expect
             if (!empty($data['name'])) {
-                return $data['name']; // e.g. "@jared:matrix.lotusguild.org"
+                $resolved = $data['name']; // e.g. "@jared:matrix.lotusguild.org"
             }
-        }
-
-        // 404 = user not found in Synapse; other codes = error
-        if ($httpCode !== 404) {
+        } elseif ($httpCode !== 404) {
+            // 404 = user not found in Synapse; other codes = error
             error_log("SynapseHelper: unexpected HTTP {$httpCode} resolving '{$username}'");
         }
 
-        return null;
+        // Memoize for the rest of this request (including negative results, so a
+        // missing/unreachable user isn't retried within the same request).
+        self::$cache[$username] = $resolved;
+        return $resolved;
     }
 
     /**
@@ -87,7 +99,16 @@ class SynapseHelper
     public static function resolveUsernames(array $usernames): array
     {
         $ids = [];
+        $deadline = microtime(true) + self::RESOLVE_BUDGET_SECONDS;
         foreach ($usernames as $username) {
+            // Cached lookups are free and always allowed; for uncached ones, stop
+            // making live calls once the batch budget is spent so a slow/unreachable
+            // Synapse can't stall the request for N × per-call timeout.
+            $cached = array_key_exists($username, self::$cache);
+            if (!$cached && microtime(true) >= $deadline) {
+                error_log('SynapseHelper: resolve budget exhausted; skipping remaining lookups');
+                break;
+            }
             $id = self::resolveUsername($username);
             if ($id !== null) {
                 $ids[] = $id;

middleware/RateLimitMiddleware.php

@@ -84,28 +84,43 @@ class RateLimitMiddleware
         $ipHash = hash('sha256', $ip . '_' . $type);
         $filePath = self::getRateLimitDir() . '/' . $ipHash . '.json';
 
-        // Load existing rate data
+        // Hold an exclusive lock across the whole read-modify-write so concurrent
+        // requests from the same IP can't both read the same count and each write
+        // count+1 (which would undercount and let the limit be exceeded).
+        $fh = @fopen($filePath, 'c+');
+        if ($fh === false) {
+            // Can't open the counter file — fail open (don't block legitimate traffic).
+            return true;
+        }
+
+        if (!flock($fh, LOCK_EX)) {
+            fclose($fh);
+            return true;
+        }
+
+        $content = stream_get_contents($fh);
         $rateData = ['count' => 0, 'window_start' => $now];
-        if (file_exists($filePath)) {
-            $content = @file_get_contents($filePath);
-            if ($content !== false) {
-                $decoded = json_decode($content, true);
-                if (is_array($decoded)) {
-                    $rateData = $decoded;
-                }
+        if ($content !== false && $content !== '') {
+            $decoded = json_decode($content, true);
+            if (is_array($decoded)) {
+                $rateData = $decoded;
             }
         }
 
-        // Check if window has expired
-        if ($now - $rateData['window_start'] >= self::WINDOW_SECONDS) {
+        // Reset when the window has expired
+        if ($now - ($rateData['window_start'] ?? $now) >= self::WINDOW_SECONDS) {
             $rateData = ['count' => 0, 'window_start' => $now];
         }
 
-        // Increment count
         $rateData['count']++;
 
-        // Save updated data
-        @file_put_contents($filePath, json_encode($rateData), LOCK_EX);
+        // Rewrite the file in place while still holding the lock
+        rewind($fh);
+        ftruncate($fh, 0);
+        fwrite($fh, json_encode($rateData));
+        fflush($fh);
+        flock($fh, LOCK_UN);
+        fclose($fh);
 
         // Check if over limit
         return $rateData['count'] <= $limit;

models/BulkOperationsModel.php

@@ -104,6 +104,11 @@ class BulkOperationsModel
                 $success = false;
 
                 try {
+                    // NOTE: bulk_status / bulk_close intentionally do NOT run
+                    // WorkflowModel::isTransitionAllowed(). Bulk operations are an
+                    // admin-only escape hatch for forcing ticket states (e.g. mass
+                    // re-opening), so they bypass the workflow transition rules that
+                    // the single-ticket update path enforces. This is by design.
                     switch ($operation['operation_type']) {
                         case 'bulk_close':
                             // Get current ticket from pre-loaded batch

models/DependencyModel.php

@@ -190,14 +190,25 @@ class DependencyModel
      */
     private function wouldCreateCycle($ticketId, $dependsOnId, $type): bool
     {
-        // Only check for cycles in blocking relationships
+        // Only blocking relationships impose an ordering that can form a cycle.
         if (!in_array($type, ['blocks', 'blocked_by'])) {
             return false;
         }
 
-        // Check if dependsOnId already has ticketId in its dependency chain
+        // Normalize the new row to a precedence edge "from must finish before to":
+        //   (t, d, 'blocks')     => t blocks d      => edge t -> d
+        //   (t, d, 'blocked_by') => t blocked_by d  => edge d -> t
+        if ($type === 'blocks') {
+            $from = $ticketId;
+            $to   = $dependsOnId;
+        } else { // blocked_by
+            $from = $dependsOnId;
+            $to   = $ticketId;
+        }
+
+        // Adding edge from->to creates a cycle iff a path to ->* from already exists.
         $visited = [];
-        return $this->hasDependencyPath($dependsOnId, $ticketId, $visited, 0);
+        return $this->hasDependencyPath($to, $from, $visited, 0);
     }
 
     /**
@@ -236,15 +247,22 @@ class DependencyModel
 
         $visited[] = $source;
 
-        $sql = "SELECT depends_on_id FROM ticket_dependencies
-                WHERE ticket_id = ? AND dependency_type IN ('blocks', 'blocked_by')";
+        // Walk the unified precedence graph forward from $source. Both directions
+        // of expression contribute an outgoing edge "$source must finish before X":
+        //   blocks rows where ticket_id=$source     -> X = depends_on_id
+        //   blocked_by rows where depends_on_id=$source -> X = ticket_id
+        $sql = "SELECT depends_on_id AS next_id FROM ticket_dependencies
+                    WHERE ticket_id = ? AND dependency_type = 'blocks'
+                UNION
+                SELECT ticket_id AS next_id FROM ticket_dependencies
+                    WHERE depends_on_id = ? AND dependency_type = 'blocked_by'";
         $stmt = $this->conn->prepare($sql);
-        $stmt->bind_param("s", $source);
+        $stmt->bind_param("ss", $source, $source);
         $stmt->execute();
         $result = $stmt->get_result();
 
         while ($row = $result->fetch_assoc()) {
-            if ($this->hasDependencyPath($row['depends_on_id'], $target, $visited, $depth + 1)) {
+            if ($this->hasDependencyPath($row['next_id'], $target, $visited, $depth + 1)) {
                 $stmt->close();
                 return true;
             }

models/TicketModel.php

@@ -208,6 +208,11 @@ class TicketModel
                 ORDER BY $sortExpression $sortDirection
                 LIMIT ? OFFSET ?";
 
+        // Keep a copy of the filter params (without LIMIT/OFFSET) for the
+        // fallback COUNT below.
+        $countParams    = $params;
+        $countParamTypes = $paramTypes;
+
         $params[] = $limit;
         $params[] = $offset;
         $paramTypes .= 'ii';
@@ -228,6 +233,24 @@ class TicketModel
         }
         $stmt->close();
 
+        // COUNT(*) OVER() rides on returned rows, so a page past the last row
+        // yields zero rows and a bogus total of 0. Fall back to a direct COUNT
+        // so the total/pages stay correct for stale or over-range page links.
+        if ($totalTickets === 0 && $offset > 0) {
+            $countSql = "SELECT COUNT(*) AS c
+                         FROM tickets t
+                         LEFT JOIN users u_created ON t.created_by = u_created.user_id
+                         LEFT JOIN users u_assigned ON t.assigned_to = u_assigned.user_id
+                         $whereClause";
+            $countStmt = $this->conn->prepare($countSql);
+            if (!empty($countParams)) {
+                $countStmt->bind_param($countParamTypes, ...$countParams);
+            }
+            $countStmt->execute();
+            $totalTickets = (int)($countStmt->get_result()->fetch_assoc()['c'] ?? 0);
+            $countStmt->close();
+        }
+
         return [
             'tickets' => $tickets,
             'total' => $totalTickets,

views/DashboardView.php

@@ -48,14 +48,14 @@ if (!empty($_GET['priority'])) {
     }
 }
 if (!empty($_GET['category'])) {
-    $activeFilters[] = ['type' => 'category', 'value' => $_GET['category'], 'label' => 'Category: ' . htmlspecialchars($_GET['category'])];
+    $activeFilters[] = ['type' => 'category', 'value' => $_GET['category'], 'label' => 'Category: ' . $_GET['category']];
 }
 if (!empty($_GET['type'])) {
-    $activeFilters[] = ['type' => 'type', 'value' => $_GET['type'], 'label' => 'Type: ' . htmlspecialchars($_GET['type'])];
+    $activeFilters[] = ['type' => 'type', 'value' => $_GET['type'], 'label' => 'Type: ' . $_GET['type']];
 }
 if (!empty($_GET['assigned_to'])) {
     $label = match ($_GET['assigned_to']) {
-        'unassigned' => 'Unassigned', 'me' => 'Me', default => 'User #' . htmlspecialchars($_GET['assigned_to'])
+        'unassigned' => 'Unassigned', 'me' => 'Me', default => 'User #' . $_GET['assigned_to']
     };
     $activeFilters[] = ['type' => 'assigned_to', 'value' => $_GET['assigned_to'], 'label' => 'Assigned: ' . $label];
 }