Quick-win fixes from second review
Security / PHP Security (semgrep) (push) Successful in 1m27s
Lint / Deploy (push) Successful in 4s
Lint / Notify on failure (push) Has been skipped
Lint / PHP (phpcs PSR-12) (push) Successful in 20s
Lint / JS (eslint) (push) Successful in 9s
Lint / PHP requirements (version + extensions) (push) Successful in 38s

- create_ticket_api.php: validate status (against TICKET_STATUSES) and
  priority (numeric 1-5). A non-numeric priority previously cast to 0 and
  escalated the ticket below P1 on the dedup/update path.
- manage_workflows.php: reject empty/invalid from_status/to_status on POST
  and PUT (must be valid ticket statuses) so the workflow table can't be
  populated with bogus transitions.
- TicketModel::getAllTickets: COUNT(*) OVER() rides on returned rows, so a
  page past the last row returned total/pages = 0. Fall back to a direct
  COUNT when an over-range page yields no rows, keeping pager math correct.
- DashboardView: stop double-escaping category/type/assigned active-filter
  labels (they were htmlspecialchars'd into the label and again at output,
  rendering R&D as R&D); output escaping is retained.
- check_duplicates.php / NotificationHelper::notifyWatchers: wrap the DB
  lookups in try/catch so a failed prepare/query degrades gracefully
  (advisory dup-check returns none; best-effort watcher notify is skipped)
  instead of fataling the request. Works whether mysqli throws or returns
  false. (manage_* endpoints already have a top-level try/catch.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-06-30 12:26:28 -04:00
co-authored by Claude Opus 4.8
parent 4164f85051
commit e0e92e326a
6 changed files with 108 additions and 33 deletions
+17 -5
View File
@@ -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
+18
View File
@@ -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']);
+16
View File
@@ -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);
+31 -25
View File
@@ -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)) {
+23
View File
@@ -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,
+3 -3
View File
@@ -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];
}