From 23a09f54fcaf26521e43f1d4ed27183e80e97eed Mon Sep 17 00:00:00 2001 From: johnnyq Date: Sat, 1 Aug 2026 15:33:48 -0400 Subject: [PATCH] Reworked getFieldById to remove escaping and and reworked everything that uses the function to also escape, was causing double escaping in many places this standardizes the function --- admin/post/contract_template.php | 6 +- admin/post/users.php | 2 +- agent/ajax.php | 4 +- agent/post/ticket.php | 10 +-- agent/tickets.php | 6 +- agent/user/user_details.php | 2 +- api/v1/ticket_replies/create.php | 2 +- functions/app.php | 104 ++++++++++++------------------- 8 files changed, 56 insertions(+), 80 deletions(-) diff --git a/admin/post/contract_template.php b/admin/post/contract_template.php index a4a30cb1..b2995c47 100644 --- a/admin/post/contract_template.php +++ b/admin/post/contract_template.php @@ -116,7 +116,7 @@ if (isset($_GET['archive_contract_template'])) { $contract_template_id = intval($_GET['archive_contract_template']); - $name = getFieldById('contract_templates', $contract_template_id, 'contract_template_name'); + $name = escapeSql(getFieldById('contract_templates', $contract_template_id, 'contract_template_name')); mysqli_query($mysqli, " UPDATE contract_templates SET contract_template_archived_at = NOW() @@ -135,7 +135,7 @@ if (isset($_GET['restore_contract_template'])) { $contract_template_id = intval($_GET['restore_contract_template']); - $name = getFieldById('contract_templates', $contract_template_id, 'contract_template_name'); + $name = escapeSql(getFieldById('contract_templates', $contract_template_id, 'contract_template_name')); mysqli_query($mysqli, " UPDATE contract_templates SET contract_template_archived_at = NULL @@ -154,7 +154,7 @@ if (isset($_GET['delete_contract_template'])) { $contract_template_id = intval($_GET['delete_contract_template']); - $name = getFieldById('contract_templates', $contract_template_id, 'contract_template_name'); + $name = escapeSql(getFieldById('contract_templates', $contract_template_id, 'contract_template_name')); mysqli_query($mysqli, " DELETE FROM contract_templates diff --git a/admin/post/users.php b/admin/post/users.php index 65991472..418263fc 100644 --- a/admin/post/users.php +++ b/admin/post/users.php @@ -278,7 +278,7 @@ if (isset($_POST['archive_user'])) { mysqli_query($mysqli, "UPDATE tickets SET ticket_assigned_to = $ticket_assign WHERE ticket_assigned_to = $user_id AND ticket_closed_at IS NULL AND ticket_resolved_at IS NULL"); mysqli_query($mysqli, "UPDATE recurring_tickets SET recurring_ticket_assigned_to = $ticket_assign WHERE recurring_ticket_assigned_to = $user_id"); - $reassigned_to_name = $ticket_assign ? escapeSql(getFieldById('users', $ticket_assign, 'user_name', 'raw')) : ''; + $reassigned_to_name = $ticket_assign ? escapeSql(getFieldById('users', $ticket_assign, 'user_name')) : ''; foreach ($affected_ticket_ids as $affected_ticket_id) { if ($reassigned_to_name) { logTicketHistory($affected_ticket_id, "$session_name reassigned the ticket to $reassigned_to_name when $user_name was archived"); diff --git a/agent/ajax.php b/agent/ajax.php index e62df700..40d7ca1d 100644 --- a/agent/ajax.php +++ b/agent/ajax.php @@ -545,7 +545,7 @@ if (isset($_POST['update_kanban_ticket'])) { mysqli_query($mysqli, "UPDATE tickets SET ticket_order = $kanban, ticket_status = $status, ticket_resolved_at = NULL WHERE ticket_id = $ticket_id"); resetTicketResolutionSla($ticket_id); syncTicketSlaClock($ticket_id); - $new_status_name = getTicketStatusName($status, 'sql'); + $new_status_name = escapeSql(getTicketStatusName($status)); logTicketHistory($ticket_id, "$session_name reopened the ticket to $new_status_name from the kanban"); triggerCustomAction('ticket_update', $ticket_id); } elseif ($status === $statuses['Resolved']) { @@ -638,7 +638,7 @@ if (isset($_POST['update_kanban_ticket'])) { // If the ticket was moved from any status to another status mysqli_query($mysqli, "UPDATE tickets SET ticket_order = $kanban, ticket_status = $status WHERE ticket_id = $ticket_id"); syncTicketSlaClock($ticket_id); - $new_status_name = getTicketStatusName($status, 'sql'); + $new_status_name = escapeSql(getTicketStatusName($status)); logTicketHistory($ticket_id, "$session_name set the status to $new_status_name from the kanban"); triggerCustomAction('ticket_update', $ticket_id); } diff --git a/agent/post/ticket.php b/agent/post/ticket.php index 0099c00e..d5a7840b 100644 --- a/agent/post/ticket.php +++ b/agent/post/ticket.php @@ -127,7 +127,7 @@ if (isset($_POST['add_ticket'])) { $ticket_details = mysqli_escape_string($mysqli, $row['ticket_details']); $ticket_priority = escapeSql($row['ticket_priority']); $ticket_status = escapeSql($row['ticket_status']); - $ticket_status_name = getTicketStatusName($row['ticket_status'], 'sql'); + $ticket_status_name = escapeSql(getTicketStatusName($row['ticket_status'])); $client_id = intval($row['ticket_client_id']); $ticket_created_by = intval($row['ticket_created_by']); $ticket_assigned_to = intval($row['ticket_assigned_to']); @@ -263,7 +263,7 @@ if (isset($_POST['edit_ticket'])) { if ($original_assigned_to !== $assigned_to) { if ($assigned_to) { - $new_agent_name = escapeSql(getFieldById('users', $assigned_to, 'user_name', 'raw')); + $new_agent_name = escapeSql(getFieldById('users', $assigned_to, 'user_name')); logTicketHistory($ticket_id, "$session_name assigned the ticket to $new_agent_name"); } else { logTicketHistory($ticket_id, "$session_name unassigned the ticket"); @@ -1548,7 +1548,7 @@ if (isset($_POST['bulk_ticket_reply'])) { mysqli_query($mysqli, "UPDATE tickets SET ticket_status = '$ticket_status' WHERE ticket_id = $ticket_id"); syncTicketSlaClock($ticket_id); - $new_status_name = getTicketStatusName($ticket_status, 'sql'); + $new_status_name = escapeSql(getTicketStatusName($ticket_status)); logTicketHistory($ticket_id, "$session_name set the status to $new_status_name"); logAudit("Ticket", "Reply", "$session_name replied to ticket $ticket_prefix$ticket_number - $ticket_subject and was a $ticket_reply_type reply", $client_id, $ticket_id); @@ -1861,7 +1861,7 @@ if (isset($_POST['add_ticket_reply'])) { } // Add Signature to the end of the ticket reply if not Internal and if there is reply if ($ticket_reply !== '' && $ticket_reply_type !== 'Internal' && $send_email == 1) { - $ticket_reply .= getFieldById('user_settings',$session_user_id,'user_config_signature', 'raw'); + $ticket_reply .= getFieldById('user_settings',$session_user_id,'user_config_signature'); } $ticket_reply = mysqli_escape_string($mysqli, $ticket_reply); // SQL Escape Ticket Reply @@ -2032,7 +2032,7 @@ if (isset($_POST['add_ticket_reply'])) { flashAlert("Stored on the ticket but too large to email: " . implode(', ', $skipped_names) . "", 'error'); } - $new_status_name = getTicketStatusName($ticket_status, 'sql'); + $new_status_name = escapeSql(getTicketStatusName($ticket_status)); logTicketHistory($ticket_id, "$session_name set the status to $new_status_name"); logAudit("Ticket", "Reply", "$session_name replied to ticket $ticket_prefix$ticket_number - $ticket_subject and was a $ticket_reply_type reply", $client_id, $ticket_id); diff --git a/agent/tickets.php b/agent/tickets.php index 6fc1fe92..a298627c 100644 --- a/agent/tickets.php +++ b/agent/tickets.php @@ -170,7 +170,7 @@ $category_filter = ''; if (!empty($_GET['category'])) { $category_filter = intval($_GET['category']); $category_query = "AND ticket_category = $category_filter"; - $active_filters[] = array('label' => 'Category', 'value' => getFieldById('categories', $category_filter, 'category_name', 'html'), 'drop' => 'category'); + $active_filters[] = array('label' => 'Category', 'value' => escapeHtml(getFieldById('categories', $category_filter, 'category_name')), 'drop' => 'category'); } // Priority Filter @@ -219,7 +219,7 @@ if (!empty($_GET['assigned'])) { } else { $ticket_assigned_filter_id = intval($_GET['assigned']); $ticket_assigned_query = "AND ticket_assigned_to = $ticket_assigned_filter_id"; - $assigned_name = $ticket_assigned_filter_id === intval($session_user_id) ? 'Me' : getFieldById('users', $ticket_assigned_filter_id, 'user_name', 'html'); + $assigned_name = $ticket_assigned_filter_id === intval($session_user_id) ? 'Me' : escapeHtml(getFieldById('users', $ticket_assigned_filter_id, 'user_name')); $active_filters[] = array('label' => 'Assigned', 'value' => $assigned_name, 'drop' => 'assigned'); } } @@ -230,7 +230,7 @@ $ticket_project_filter_id = ''; if (!empty($_GET['project'])) { $ticket_project_filter_id = intval($_GET['project']); $ticket_project_snippet = "AND ticket_project_id = $ticket_project_filter_id"; - $active_filters[] = array('label' => 'Project', 'value' => getFieldById('projects', $ticket_project_filter_id, 'project_name', 'html'), 'drop' => 'project'); + $active_filters[] = array('label' => 'Project', 'value' => escapeHtml(getFieldById('projects', $ticket_project_filter_id, 'project_name')), 'drop' => 'project'); } // Date range - filter_header.php resolves the canned ranges into $dtf / $dtt diff --git a/agent/user/user_details.php b/agent/user/user_details.php index 63afce25..f3003aeb 100644 --- a/agent/user/user_details.php +++ b/agent/user/user_details.php @@ -58,7 +58,7 @@ require_once "includes/inc_all_user.php";
-
diff --git a/api/v1/ticket_replies/create.php b/api/v1/ticket_replies/create.php index b94184b6..009443ac 100644 --- a/api/v1/ticket_replies/create.php +++ b/api/v1/ticket_replies/create.php @@ -87,7 +87,7 @@ if (!empty($ticket_id) && !empty($reply)) { if (!empty($reply_ticket_status)) { mysqli_query($mysqli, "UPDATE tickets SET ticket_status = $reply_ticket_status WHERE ticket_id = $ticket_id LIMIT 1"); - $new_status_name = getTicketStatusName($reply_ticket_status, 'sql'); + $new_status_name = escapeSql(getTicketStatusName($reply_ticket_status)); logTicketHistory($ticket_id, "Status set to $new_status_name via the API ($api_key_name)"); // Resolve the ticket, if set diff --git a/functions/app.php b/functions/app.php index 1e90d82c..710538cc 100644 --- a/functions/app.php +++ b/functions/app.php @@ -56,14 +56,10 @@ function getInvoiceBadgeColor($invoice_status) { } /* - * The display name for a ticket status id. - * - * $escape_method follows getFieldById() - 'html' (the default, unchanged for - * existing callers), 'sql' for interpolating into a query, or 'raw'. The one - * caller that wanted SQL was wrapping this in escapeSql(), which escaped the - * already-HTML-escaped string and mangled names containing & or an apostrophe. + * The display name for a ticket status id, RAW. Escaping is the caller's job - + * same convention as getFieldById() above. */ -function getTicketStatusName($ticket_status, $escape_method = 'html') { +function getTicketStatusName($ticket_status) { global $mysqli; @@ -75,18 +71,11 @@ function getTicketStatusName($ticket_status, $escape_method = 'html') { return "Unknown"; } - if ($escape_method === 'sql') { - return escapeSql($row['ticket_status_name']); - } - - if ($escape_method === 'raw') { - return $row['ticket_status_name']; - } - - return escapeHtml($row['ticket_status_name']); + return $row['ticket_status_name']; } + /** * Copies a ticket template's tasks onto a ticket. * @@ -201,19 +190,21 @@ function parseSubmittedTasks() { return $tasks; } -/** - * Retrieves a specified field's value from a table based on the record's id. - * It validates the table and field names, automatically determines the primary key (or uses the first column as fallback), - * and returns the field value with an appropriate escaping method. +/* + * Fetches one field from one row by id, and returns it RAW. * - * @param string $table The name of the table. - * @param int $id The record's id. - * @param string $field The field (column) to retrieve. - * @param string $escape_method The escape method: 'sql' (default, auto-detects int), 'html', 'json', or 'int'. + * Escaping is the caller's job, the same as any other value read out of the + * database - wrap the call in escapeSql() for a query or escapeHtml() for + * output. This function used to escape for you via an $escape_method argument, + * which meant half its callers wrapped it in escapeSql() anyway and got a + * double-escaped value: a client named O'Brien came back as O\'Brien and the + * backslash ended up in export filenames, flash messages and, on the user + * restore path, written back into the database. * - * @return mixed The escaped field value, or null if not found or invalid input. + * Table, field and id are still validated here - that is about building a safe + * query, not about escaping what comes out of it. */ -function getFieldById($table, $id, $field, $escape_method = 'sql') { +function getFieldById($table, $id, $field) { global $mysqli; // Use the global MySQLi connection // Validate table and field names to allow only letters, numbers, and underscores @@ -224,29 +215,34 @@ function getFieldById($table, $id, $field, $escape_method = 'sql') { // Sanitize id as an integer $id = (int)$id; - // Get the list of columns and their details from the table - $columns_result = mysqli_query($mysqli, "SHOW COLUMNS FROM `$table`"); + /* + * Get the list of columns from the table, to find the primary key and to + * confirm the requested field exists. + * + * The catch is what makes the "table not found" case actually return null: + * mysqli throws on an unknown table by default on PHP 8.1+, so this + * function's own not-found branch was unreachable and a bad table name + * took the whole page down instead. + */ + try { + $columns_result = mysqli_query($mysqli, "SHOW COLUMNS FROM `$table`"); + } catch (mysqli_sql_exception $e) { + return null; // Table not found + } + if (!$columns_result || mysqli_num_rows($columns_result) == 0) { return null; // Table not found or has no columns } - // Build an associative array with column details $columns = []; - while ($row = mysqli_fetch_assoc($columns_result)) { - $columns[$row['Field']] = [ - 'type' => $row['Type'], - 'key' => $row['Key'] - ]; - } - - // Find the primary key field if available $id_field = null; - foreach ($columns as $col => $details) { - if ($details['key'] === 'PRI') { - $id_field = $col; - break; + while ($row = mysqli_fetch_assoc($columns_result)) { + $columns[$row['Field']] = true; + if (!$id_field && $row['Key'] === 'PRI') { + $id_field = $row['Field']; } } + // Fallback: if no primary key is found, use the first column if (!$id_field) { reset($columns); @@ -259,37 +255,17 @@ function getFieldById($table, $id, $field, $escape_method = 'sql') { } // Build and execute the query to fetch the specified field value - $query = "SELECT `$field` FROM `$table` WHERE `$id_field` = $id"; - $sql = mysqli_query($mysqli, $query); + $sql = mysqli_query($mysqli, "SELECT `$field` FROM `$table` WHERE `$id_field` = $id"); if ($sql && mysqli_num_rows($sql) > 0) { $row = mysqli_fetch_assoc($sql); - $value = $row[$field]; - - // Apply the desired escaping method or auto-detect integer type if using SQL escaping - switch ($escape_method) { - case 'raw': - return $value; // Return as-is from the database - case 'html': - return htmlspecialchars($value ?? '', ENT_QUOTES, 'UTF-8'); // Escape for HTML - case 'json': - return json_encode($value); // Escape for JSON - case 'int': - return (int)$value; // Explicitly cast value to integer - case 'sql': - default: - // Auto-detect if the field type is integer - if (stripos($columns[$field]['type'], 'int') !== false) { - return (int)$value; - } else { - return escapeSql($value); // Escape for SQL using a custom function - } - } + return $row[$field]; } return null; // Return null if no record was found } + // Recursive function to display folder options - Used in folders files and documents function displayFolderOptions($parent_folder_id, $client_id, $indent = 0) { global $mysqli;