diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f6b8a72d0..cf79c076b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -146,7 +146,7 @@ ITFlow does not use prepared statements or an ORM; queries are built as strings. - **Integers** (IDs, flags, counts): `intval($_POST['ticket_id'])`. Interpolate unquoted. - **Strings**: `escapeSql($_POST['subject'])`. This normalizes encoding to UTF-8, then runs `strip_tags()`, `trim()`, and `mysqli_real_escape_string()`. Because it relies on SQL escaping, the value **must be placed inside quotes in the query** (`'$subject'`). An escaped string interpolated without quotes is still injectable. - **Values read back from the database** get the same treatment before reuse in another query (you will see `escapeSql($row['ticket_prefix'])` throughout — this is why). -- **`logAudit()`, `appNotify()`, `logHistory()` and `logTicketHistory()` are queries too.** They interpolate their `$description` / `$details` / `$status` arguments straight into an `INSERT` — the SQL is just hidden inside the helper. A DB-read value passed into one of them (`logAudit("Asset", "Delete", "$asset_name ...", ...)`) must be `escapeSql`'d first, exactly as if you had written the `INSERT` by hand. This is easy to miss precisely because the call doesn't *look* like a query. +- **`logAudit()`, `appNotify()`, `logHistory()` and `logTicketHistory()` are queries too.** They interpolate their `$description` / `$details` / `$status` arguments straight into an `INSERT` — the SQL is just hidden inside the helper. A DB-read value passed into one of them (`logAudit("Asset", "Delete", "$asset_name ...", ...)`) must be `escapeSql`'d first, exactly as if you had written the `INSERT` by hand. This is easy to miss precisely because the call doesn't *look* like a query. Note that `escapeHtml()` is **not** a substitute here: it encodes `'` and `"` so it happens to block a quote-breakout, but it leaves backslashes untouched, so a value ending in `\` still escapes the closing quote. All four sinks now trim an odd trailing backslash as a backstop, but the value still owes `escapeSql` — the guard is defence-in-depth, not the fix. If you write a query and even one variable in it skipped these, that is a SQL injection. This is the single most common review rejection. **Fetch helpers return raw values — you escape them.** `getFieldById()` and `getTicketStatusName()` hand back exactly what is in the column. Escaping is the call site's job, the same as any other row you read: diff --git a/admin/post/saved_payment_method.php b/admin/post/saved_payment_method.php index 51f6687d5..bad78f3ca 100644 --- a/admin/post/saved_payment_method.php +++ b/admin/post/saved_payment_method.php @@ -61,7 +61,8 @@ if (isset($_GET['delete_saved_payment'])) { // SQL Cascade delete will Remove All Associated Auto Payment Methods on recurring invoices in the recurring payments table. - logAudit("Payment Provider", "Update", "$session_name deleted saved payment method $saved_payment_description (PM: $payment_method)", $client_id); + $payment_method_esc = escapeSql($payment_method); + logAudit("Payment Provider", "Update", "$session_name deleted saved payment method $saved_payment_description (PM: $payment_method_esc)", $client_id); flashAlert("Payment method $saved_payment_description removed", 'error'); diff --git a/agent/post/task.php b/agent/post/task.php index ab7d1f45e..d0a8765cd 100644 --- a/agent/post/task.php +++ b/agent/post/task.php @@ -351,7 +351,7 @@ if (isset($_GET['approve_ticket_task'])) { $approval_row = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT approval_created_by, approval_required_user_id, approval_scope, approval_type, task_name, task_ticket_id FROM task_approvals LEFT JOIN tasks on task_id = approval_task_id WHERE approval_id = $approval_id AND approval_task_id = $task_id AND approval_scope = 'internal'")); - $task_name = escapeHtml($approval_row['task_name']); + $task_name = escapeSql($approval_row['task_name']); $scope = escapeHtml($approval_row['approval_scope']); $type = escapeHtml($approval_row['approval_type']); $required_user = intval($approval_row['approval_required_user_id']); diff --git a/client/post.php b/client/post.php index 45bdd51e6..8d527f435 100644 --- a/client/post.php +++ b/client/post.php @@ -277,7 +277,7 @@ if (isset($_GET['approve_ticket_task'])) { $approval_row = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT approval_created_by, approval_required_user_id, approval_scope, approval_type, task_name, task_ticket_id FROM task_approvals LEFT JOIN tasks on task_id = approval_task_id WHERE approval_id = $approval_id AND approval_task_id = $task_id AND approval_url_key = '$url_key' AND approval_status = 'pending' AND approval_scope = 'client'")); - $task_name = escapeHtml($approval_row['task_name']); + $task_name = escapeSql($approval_row['task_name']); $scope = escapeHtml($approval_row['approval_scope']); $type = escapeHtml($approval_row['approval_type']); $required_user = intval($approval_row['approval_required_user_id']); diff --git a/functions/logging.php b/functions/logging.php index f14138e1e..c2f73b12f 100644 --- a/functions/logging.php +++ b/functions/logging.php @@ -22,10 +22,26 @@ function appNotify($type, $details, $action = null, $client_id = 0, $entity_id = $action = "NULL"; // Without quotes for SQL NULL } + $client_id = intval($client_id); + $entity_id = intval($entity_id); + $type = substr($type, 0, 200); $details = substr($details, 0, 1000); $action = substr($action, 0, 250); + // Callers pass values that are already SQL-safe, but cutting at a fixed + // length (or an escaper that leaves backslashes, e.g. escapeHtml) can leave + // an odd trailing backslash that would escape this query's closing quote + if ((strlen($type) - strlen(rtrim($type, '\\'))) % 2 === 1) { + $type = substr($type, 0, -1); + } + if ((strlen($details) - strlen(rtrim($details, '\\'))) % 2 === 1) { + $details = substr($details, 0, -1); + } + if ((strlen($action) - strlen(rtrim($action, '\\'))) % 2 === 1) { + $action = substr($action, 0, -1); + } + $sql = mysqli_query($mysqli, "SELECT user_id FROM users WHERE user_type = 1 AND user_status = 1 AND user_archived_at IS NULL "); @@ -52,6 +68,19 @@ function logAudit($type, $action, $description, $client_id = 0, $entity_id = 0) $action = substr($action, 0, 255); $description = substr($description, 0, 1000); + // Callers pass values that are already SQL-safe, but cutting at a fixed + // length (or an escaper that leaves backslashes, e.g. escapeHtml) can leave + // an odd trailing backslash that would escape this query's closing quote + if ((strlen($type) - strlen(rtrim($type, '\\'))) % 2 === 1) { + $type = substr($type, 0, -1); + } + if ((strlen($action) - strlen(rtrim($action, '\\'))) % 2 === 1) { + $action = substr($action, 0, -1); + } + if ((strlen($description) - strlen(rtrim($description, '\\'))) % 2 === 1) { + $description = substr($description, 0, -1); + } + mysqli_query($mysqli, "INSERT INTO logs SET log_type = '$type', log_action = '$action', log_description = '$description', log_ip = '$session_ip', log_user_agent = '$session_user_agent', log_client_id = $client_id, log_user_id = $session_user_id, log_entity_id = $entity_id"); } diff --git a/guest/guest_post.php b/guest/guest_post.php index 35f01ad5c..a28e8c47c 100644 --- a/guest/guest_post.php +++ b/guest/guest_post.php @@ -260,7 +260,7 @@ if (isset($_GET['approve_ticket_task'])) { $approval_row = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT approval_created_by, approval_required_user_id, approval_scope, approval_type, task_name, task_ticket_id FROM task_approvals LEFT JOIN tasks on task_id = approval_task_id WHERE approval_id = $approval_id AND approval_task_id = $task_id AND approval_url_key = '$url_key' AND approval_status = 'pending'")); - $task_name = escapeHtml($approval_row['task_name']); + $task_name = escapeSql($approval_row['task_name']); $scope = escapeHtml($approval_row['approval_scope']); $type = escapeHtml($approval_row['approval_type']); $required_user = intval($approval_row['approval_required_user_id']);