LotusGuild/tinker_tickets
4
0
Fork 0
Code Issues 6 Pull Requests Actions Packages Projects Releases Wiki Activity

Fix API security: dependency/visibility leaks, authz, CSRF, comment spoofing

Browse Source 
- ticket_dependencies.php: pass current user id/groups/is_admin into the
  visibility-filtered DependencyModel methods; drop (int) casts that
  stripped leading zeros from varchar ticket_ids
- update_ticket.php: authorize visibility changes (admin or creator only);
  enforce requires_comment transitions server-side (400 + requires_comment
  flag so the client can prompt-and-retry); return proper 401/400/403
- add_comment.php: take commenter name from the session not the client
  (anti-spoofing); validate parent_comment_id belongs to the ticket;
  reject empty comments; pass ticket visibility to notifications so
  non-public comment bodies aren't leaked
- add_comment/update_comment/bulk_operation: validate CSRF for all
  state-changing methods, not just POST
- bootstrap.php: return the current CSRF token on rejection and never
  rotate it on a rejected request, so a desynced client can auto-recover
- correct auth->401 and validation->400 status codes across these endpoints

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
jared
2026-07-10 11:48:34 -04:00
co-authored by Claude Opus 4.8
parent c5f7a01e1d
commit 327c225ded
7 changed files with 152 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/add_comment.php
+49 -6
View File
@@ -38,12 +38,16 @@ try {
session_start();
}
if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
throw new Exception("Authentication required");
ob_end_clean();
http_response_code(401);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Authentication required']);
exit;
}
// CSRF Protection
// CSRF Protection for all state-changing methods (any non-GET/HEAD request)
require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) {
$csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? '';
if (!CsrfMiddleware::validateToken($csrfToken)) {
http_response_code(403);
@@ -63,7 +67,11 @@ try {
$data = json_decode(file_get_contents('php://input'), true);
if (!$data) {
throw new Exception("Invalid JSON data received");
http_response_code(400);
ob_end_clean();
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Invalid JSON data received']);
exit;
}
$ticketId = isset($data['ticket_id']) ? trim((string)$data['ticket_id']) : '';
@@ -75,6 +83,20 @@ try {
exit;
}
// Reject empty/whitespace-only comments
$commentTextRaw = isset($data['comment_text']) ? trim((string)$data['comment_text']) : '';
if ($commentTextRaw === '') {
http_response_code(400);
ob_end_clean();
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Comment text cannot be empty']);
exit;
}
// Never trust a client-supplied display name — always attribute the comment to
// the authenticated session user.
$data['user_name'] = $currentUser['display_name'] ?? $currentUser['username'] ?? 'User';
// Verify user can access the ticket before allowing a comment
$ticketModel = new TicketModel($conn);
$ticket = $ticketModel->getTicketById($ticketId);
@@ -97,6 +119,18 @@ try {
$commentModel = new CommentModel($conn);
$auditLog = new AuditLogModel($conn);
// If replying, the parent comment must belong to this same (accessible) ticket.
if (isset($data['parent_comment_id']) && $data['parent_comment_id'] !== null && $data['parent_comment_id'] !== '') {
$parentComment = $commentModel->getCommentById((int)$data['parent_comment_id']);
if (!$parentComment || (string)$parentComment['ticket_id'] !== (string)$ticketId) {
http_response_code(400);
ob_end_clean();
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Invalid parent comment']);
exit;
}
}
// Extract @mentions from comment text
$mentions = $commentModel->extractMentions($data['comment_text'] ?? '');
$mentionedUsers = [];
@@ -130,6 +164,7 @@ try {
$authorDisplay = $currentUser['display_name'] ?? $currentUser['username'] ?? null;
$commentText = $data['comment_text'] ?? '';
$ticketTitle = $ticket['title'] ?? "Ticket #{$ticketId}";
$ticketVisibility = $ticket['visibility'] ?? 'public';
// @mention notifications — resolve usernames → Matrix IDs via Synapse Admin API
if (!empty($mentionedUsers)) {
@@ -142,7 +177,14 @@ try {
// General comment notification (opt-in via MATRIX_NOTIFY_COMMENTS)
if (!empty($GLOBALS['config']['MATRIX_NOTIFY_COMMENTS'])) {
NotificationHelper::sendCommentNotification($ticketId, $ticketTitle, $commentText, $authorDisplay);
NotificationHelper::sendCommentNotification(
$ticketId,
$ticketTitle,
$commentText,
$authorDisplay,
$ticketVisibility !== 'public',
$ticketVisibility
);
}
// Notify watchers of the new comment
@@ -152,7 +194,8 @@ try {
$ticketTitle,
'comment_added',
['author' => $authorDisplay, 'preview' => mb_strimwidth($commentText, 0, 200, '…')],
(int)$userId
(int)$userId,
$ticketVisibility
);
// Add mentioned users to result for frontend
api/bootstrap.php
+8 -1
View File
@@ -34,9 +34,16 @@ if (in_array($_SERVER['REQUEST_METHOD'], ['POST', 'PUT', 'DELETE'])) {
require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php';
$csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? '';
if (!CsrfMiddleware::validateToken($csrfToken)) {
// Do NOT rotate on a rejected request. Return the current valid token so a
// client whose token drifted out of sync can recover on its next request
// (the response body is same-origin only, so this can't aid a CSRF attacker).
http_response_code(403);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Invalid CSRF token']);
echo json_encode([
'success' => false,
'error' => 'Invalid CSRF token',
'csrf_token' => CsrfMiddleware::getToken()
]);
exit;
}
// Rotate token after successful validation; endpoints include it in their JSON response
api/bulk_operation.php
+4 -2
View File
@@ -19,9 +19,9 @@ if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
exit;
}
// CSRF Protection
// CSRF Protection for all state-changing methods (any non-GET/HEAD request)
require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) {
$csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? '';
if (!CsrfMiddleware::validateToken($csrfToken)) {
http_response_code(403);
@@ -47,6 +47,7 @@ $parameters = $data['parameters'] ?? null;
// Validate input
$validOperationTypes = ['bulk_close', 'bulk_assign', 'bulk_priority', 'bulk_status', 'bulk_delete'];
if (!$operationType || !in_array($operationType, $validOperationTypes, true) || empty($ticketIds)) {
http_response_code(400);
echo json_encode(['success' => false, 'error' => 'Operation type and ticket IDs required']);
exit;
}
@@ -57,6 +58,7 @@ $ticketIds = array_values(array_filter(array_map(function ($id) {
return (ctype_digit($s) && (int)$s > 0) ? $s : null;
}, $ticketIds)));
if (empty($ticketIds)) {
http_response_code(400);
echo json_encode(['success' => false, 'error' => 'No valid ticket IDs provided']);
exit;
}
api/delete_comment.php
+10 -2
View File
@@ -36,7 +36,11 @@ try {
session_start();
}
if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
throw new Exception("Authentication required");
ob_end_clean();
http_response_code(401);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Authentication required']);
exit;
}
// CSRF Protection
@@ -64,7 +68,11 @@ try {
if (isset($_POST['comment_id'])) {
$data = ['comment_id' => $_POST['comment_id']];
} else {
throw new Exception("Missing required field: comment_id");
ob_end_clean();
http_response_code(400);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Missing required field: comment_id']);
exit;
}
}
api/ticket_dependencies.php
+10 -7
View File
@@ -80,6 +80,9 @@ if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
$userId = $_SESSION['user']['user_id'];
$currentUser = $_SESSION['user'];
$isAdmin = $currentUser['is_admin'] ?? false;
// users.groups is a comma-separated string; the dependency model expects an array.
$userGroups = array_values(array_filter(array_map('trim', explode(',', $currentUser['groups'] ?? ''))));
// CSRF Protection for POST/DELETE
if ($_SERVER['REQUEST_METHOD'] === 'POST' || $_SERVER['REQUEST_METHOD'] === 'DELETE') {
@@ -121,14 +124,14 @@ try {
}
// Verify user can access this ticket
$ticket = $ticketModel->getTicketById((int)$ticketId);
$ticket = $ticketModel->getTicketById($ticketId);
if (!$ticket || !$ticketModel->canUserAccessTicket($ticket, $currentUser)) {
ResponseHelper::notFound('Ticket not found');
}
try {
$dependencies = $dependencyModel->getDependencies($ticketId);
$dependents = $dependencyModel->getDependentTickets($ticketId);
$dependencies = $dependencyModel->getDependencies($ticketId, $userId, $userGroups, $isAdmin);
$dependents = $dependencyModel->getDependentTickets($ticketId, $userId, $userGroups, $isAdmin);
} catch (Exception $e) {
error_log('Query error in ticket_dependencies.php GET: ' . $e->getMessage());
ResponseHelper::serverError('Failed to retrieve dependencies');
@@ -157,11 +160,11 @@ try {
}
// Verify user can access both tickets before creating dependency
$srcTicket = $ticketModel->getTicketById((int)$ticketId);
$srcTicket = $ticketModel->getTicketById($ticketId);
if (!$srcTicket || !$ticketModel->canUserAccessTicket($srcTicket, $currentUser)) {
ResponseHelper::notFound('Ticket not found');
}
$tgtTicket = $ticketModel->getTicketById((int)$dependsOnId);
$tgtTicket = $ticketModel->getTicketById($dependsOnId);
if (!$tgtTicket || !$ticketModel->canUserAccessTicket($tgtTicket, $currentUser)) {
ResponseHelper::notFound('Target ticket not found');
}
@@ -205,7 +208,7 @@ try {
}
// Verify user can access the source ticket
$srcTicket = $ticketModel->getTicketById((int)$ticketId);
$srcTicket = $ticketModel->getTicketById($ticketId);
if (!$srcTicket || !$ticketModel->canUserAccessTicket($srcTicket, $currentUser)) {
ResponseHelper::notFound('Ticket not found');
}
@@ -235,7 +238,7 @@ try {
ResponseHelper::notFound('Dependency not found');
}
$depTicket = $ticketModel->getTicketById((int)$depRow['ticket_id']);
$depTicket = $ticketModel->getTicketById($depRow['ticket_id']);
if (!$depTicket || !$ticketModel->canUserAccessTicket($depTicket, $currentUser)) {
ResponseHelper::forbidden('Access denied');
}
api/update_comment.php
+17 -5
View File
@@ -27,12 +27,16 @@ try {
session_start();
}
if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
throw new Exception("Authentication required");
ob_end_clean();
http_response_code(401);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Authentication required']);
exit;
}
// CSRF Protection
// CSRF Protection for all state-changing methods (any non-GET/HEAD request)
require_once dirname(__DIR__) . '/middleware/CsrfMiddleware.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST' || $_SERVER['REQUEST_METHOD'] === 'PUT') {
if (!in_array($_SERVER['REQUEST_METHOD'], ['GET', 'HEAD'], true)) {
$csrfToken = $_SERVER['HTTP_X_CSRF_TOKEN'] ?? '';
if (!CsrfMiddleware::validateToken($csrfToken)) {
http_response_code(403);
@@ -53,7 +57,11 @@ try {
$data = json_decode(file_get_contents('php://input'), true);
if (!$data || !isset($data['comment_id']) || !isset($data['comment_text'])) {
throw new Exception("Missing required fields: comment_id, comment_text");
ob_end_clean();
http_response_code(400);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Missing required fields: comment_id, comment_text']);
exit;
}
$commentId = (int)$data['comment_id'];
@@ -61,7 +69,11 @@ try {
$markdownEnabled = isset($data['markdown_enabled']) && $data['markdown_enabled'];
if (empty($commentText)) {
throw new Exception("Comment text cannot be empty");
ob_end_clean();
http_response_code(400);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Comment text cannot be empty']);
exit;
}
// Initialize models
api/update_ticket.php
+54 -6
View File
@@ -34,7 +34,11 @@ try {
session_start();
}
if (!isset($_SESSION['user']) || !isset($_SESSION['user']['user_id'])) {
throw new Exception("Authentication required");
ob_end_clean();
http_response_code(401);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Authentication required']);
exit;
}
// CSRF Protection
@@ -115,7 +119,8 @@ try {
if (empty($updateData['title'])) {
return [
'success' => false,
'error' => 'Title cannot be empty'
'error' => 'Title cannot be empty',
'http_status' => 400
];
}
@@ -123,7 +128,8 @@ try {
if ($updateData['priority'] < 1 || $updateData['priority'] > 5) {
return [
'success' => false,
'error' => 'Priority must be between 1 and 5'
'error' => 'Priority must be between 1 and 5',
'http_status' => 400
];
}
@@ -137,11 +143,32 @@ try {
$visibilityGroups = implode(',', array_map('trim', $visibilityGroups));
}
// Authorization: only an admin or the ticket's creator may change
// visibility. Enforce only when the requested visibility actually
// differs so ordinary edits that re-send the same value aren't blocked.
$currentVisibility = $currentTicket['visibility'] ?? 'public';
$currentGroups = $currentTicket['visibility_groups'] ?? null;
$groupsProvided = array_key_exists('visibility_groups', $data);
$visibilityChanged = ($data['visibility'] !== $currentVisibility)
|| ($groupsProvided && (string)$visibilityGroups !== (string)$currentGroups);
if ($visibilityChanged) {
$isCreator = $this->userId !== null
&& (int)($currentTicket['created_by'] ?? 0) === (int)$this->userId;
if (!$this->isAdmin && !$isCreator) {
return [
'success' => false,
'error' => 'You do not have permission to change ticket visibility',
'http_status' => 403
];
}
}
// Internal visibility requires at least one group
if ($data['visibility'] === 'internal' && (empty($visibilityGroups) || trim($visibilityGroups) === '')) {
return [
'success' => false,
'error' => 'Internal visibility requires at least one group to be specified'
'error' => 'Internal visibility requires at least one group to be specified',
'http_status' => 400
];
}
}
@@ -160,6 +187,19 @@ try {
'error' => 'Status transition not allowed: ' . $currentTicket['status'] . ' → ' . $updateData['status']
];
}
// Enforce requires_comment transitions server-side.
if ($this->workflowModel->transitionRequiresComment($currentTicket['status'], $updateData['status'])) {
$comment = trim((string)($data['comment'] ?? $data['comment_text'] ?? ''));
if ($comment === '') {
return [
'success' => false,
'error' => 'A comment is required for this status change',
'requires_comment' => true,
'http_status' => 400
];
}
}
}
// Update ticket with user tracking and optional optimistic locking
@@ -257,11 +297,19 @@ try {
$data = json_decode($input, true);
if (!$data) {
throw new Exception("Invalid JSON data received: " . $input);
ob_end_clean();
http_response_code(400);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Invalid JSON data received']);
exit;
}
if (!isset($data['ticket_id'])) {
throw new Exception("Missing ticket_id parameter");
ob_end_clean();
http_response_code(400);
header('Content-Type: application/json');
echo json_encode(['success' => false, 'error' => 'Missing ticket_id parameter']);
exit;
}
$ticketId = trim((string)$data['ticket_id']);