From 156c51224d42126469cbade0391d3eaa9611600f Mon Sep 17 00:00:00 2001 From: johnnyq Date: Fri, 28 Aug 2026 16:38:42 -0400 Subject: [PATCH] Fix client portal review findings: PIN wipe, log indexing, statement currency (db 2.7.8) Setting a PIN containing < or > silently cleared it: the length check ran before escapeSql(), whose strip_tags() then emptied the value, and the UPDATE stored the blank while flashing success. Length is now checked after sanitising. Password and PIN changes require the current password. SSO contacts are exempt - no local password to check, and the IdP already did it. New index on logs(log_user_id, log_client_id) for the portal profile and activity pages, which were scanning the whole table twice per profile view. admin/audit_logs.php's date filter rewritten as a half-open range so KEY log_created_at is usable - DATE(log_created_at) BETWEEN made it non-sargable. Portal statement page and PDF now render in the client's currency, matching the guest view and the emailed statement. Quick Send asks for confirmation; confirm-link extended to submit buttons. Portal audit entries logged an empty name - client/post.php used , which only exists agent-side. --- admin/audit_logs.php | 10 +++++- admin/database_updates/2.7.8.php | 38 +++++++++++++++++++++ agent/invoice.php | 10 ++++-- agent/invoices.php | 5 ++- agent/quote.php | 10 ++++-- agent/quotes.php | 5 ++- client/functions.php | 30 +++++++++++++++++ client/post.php | 58 +++++++++++++++++++++----------- client/profile.php | 20 +++++++++++ client/statement.php | 20 ++++++++--- db.sql | 3 +- js/confirm_modal.js | 40 +++++++++++++++++++--- 12 files changed, 213 insertions(+), 36 deletions(-) create mode 100644 admin/database_updates/2.7.8.php diff --git a/admin/audit_logs.php b/admin/audit_logs.php index ae0b3c44d..d69f5ac54 100644 --- a/admin/audit_logs.php +++ b/admin/audit_logs.php @@ -46,6 +46,14 @@ if (isset($_GET['action']) & !empty($_GET['action'])) { $action_filter = ''; } +/* + * The date filter is a half-open range rather than DATE(log_created_at) BETWEEN + * ... - wrapping the column in a function makes the comparison non-sargable, so + * KEY log_created_at could never be used and every page view scanned the whole + * table. '< dtt + 1 day' keeps the end date inclusive, which BETWEEN on a bare + * DATETIME would not: '2026-08-28' alone means midnight, and would drop that + * day's rows. + */ $sql = mysqli_query( $mysqli, "SELECT SQL_CALC_FOUND_ROWS client_id, client_name, log_action, log_created_at, log_description, log_entity_id, log_id, @@ -53,7 +61,7 @@ $sql = mysqli_query( LEFT JOIN users ON log_user_id = user_id LEFT JOIN clients ON log_client_id = client_id WHERE (log_type LIKE '%$q%' OR log_action LIKE '%$q%' OR log_description LIKE '%$q%' OR log_ip LIKE '%$q%' OR log_user_agent LIKE '%$q%' OR user_name LIKE '%$q%' OR client_name LIKE '%$q%') - AND DATE(log_created_at) BETWEEN '$dtf' AND '$dtt' + AND log_created_at >= '$dtf 00:00:00' AND log_created_at < DATE_ADD('$dtt', INTERVAL 1 DAY) $user_query $client_query $log_type_query diff --git a/admin/database_updates/2.7.8.php b/admin/database_updates/2.7.8.php new file mode 100644 index 000000000..7f7518748 --- /dev/null +++ b/admin/database_updates/2.7.8.php @@ -0,0 +1,38 @@ +
0) { ?> - @@ -318,7 +321,10 @@ if (isset($_GET['invoice_id'])) { Packing Slip 0) { ?> - diff --git a/agent/invoices.php b/agent/invoices.php index cacaf1f72..9f56b88d7 100644 --- a/agent/invoices.php +++ b/agent/invoices.php @@ -441,7 +441,10 @@ $num_rows = mysqli_fetch_row(mysqli_query($mysqli, "SELECT FOUND_ROWS()"));
- diff --git a/agent/quote.php b/agent/quote.php index 5fe6dc91e..6b008a83a 100644 --- a/agent/quote.php +++ b/agent/quote.php @@ -168,7 +168,10 @@ if (isset($_GET['quote_id'])) {
0) { ?> - @@ -237,7 +240,10 @@ if (isset($_GET['quote_id'])) { Download PDF 0) { ?> - diff --git a/agent/quotes.php b/agent/quotes.php index 3bc54a678..c624be43a 100644 --- a/agent/quotes.php +++ b/agent/quotes.php @@ -224,7 +224,10 @@ $num_rows = mysqli_fetch_row(mysqli_query($mysqli, "SELECT FOUND_ROWS()"));
- diff --git a/client/functions.php b/client/functions.php index b637d4d5a..5aa14f994 100644 --- a/client/functions.php +++ b/client/functions.php @@ -72,6 +72,36 @@ function enforceContactCan($capability) { } } +/* + * Confirms the person at the keyboard is the account holder, before a change + * that would let someone who hijacked a session take the account over or + * defeat phone verification. + * + * Returns true for SSO contacts without checking anything: there is no local + * password to compare against, and the identity provider has already done this + * work. Gating them on a password they do not have would just lock them out. + */ +function portalReauthenticate($current_password) { + global $mysqli, $session_user_id; + + if (($_SESSION['login_method'] ?? 'local') !== 'local') { + return true; + } + + if (empty($current_password)) { + return false; + } + + $sql = mysqli_query($mysqli, "SELECT user_password FROM users WHERE user_id = $session_user_id LIMIT 1"); + $row = mysqli_fetch_assoc($sql); + + if (!$row || empty($row['user_password'])) { + return false; + } + + return password_verify($current_password, $row['user_password']); +} + /* * A timestamp a person can read, in the company's configured date and time * format rather than the raw DATETIME the database hands back. diff --git a/client/post.php b/client/post.php index a079f644f..45bdd51e6 100644 --- a/client/post.php +++ b/client/post.php @@ -232,21 +232,27 @@ if (isset($_POST['set_contact_pin'])) { * section, so every signed-in contact manages their own - same as the * password change above. */ - $pin = trim($_POST['pin'] ?? ''); - - if ($pin === '') { - flashAlert("Enter a PIN, or leave the page to keep the current one", 'error'); - redirect('profile.php'); - } - - if (strlen($pin) < 4) { - flashAlert("Your PIN needs to be at least 4 characters", 'error'); + // The PIN is what we read back to verify a caller, so changing it is + // re-authenticated for password logins. SSO contacts are exempt: they have + // no local password to check, and the identity provider already did this. + if (!portalReauthenticate($_POST['current_password'] ?? '')) { + flashAlert("That password was not right - your PIN has not been changed", 'error'); redirect('profile.php'); } // contact_pin is varchar(255) - trim to fit rather than let an over-long - // value error out under strict mode - $pin = escapeSql(substr($pin, 0, 255)); + // value error out under strict mode. + // + // escapeSql() runs strip_tags() before escaping, so the length has to be + // re-checked AFTER it: a PIN of "<1234>" passed a check on the raw input, + // came out of strip_tags() as an empty string, and the UPDATE below then + // silently WIPED the contact's PIN while flashing "Phone PIN updated". + $pin = escapeSql(substr(trim($_POST['pin'] ?? ''), 0, 255)); + + if (strlen($pin) < 4) { + flashAlert("Your PIN needs to be at least 4 characters, and cannot contain < or >", 'error'); + redirect('profile.php'); + } mysqli_query($mysqli, "UPDATE contacts SET contact_pin = '$pin' WHERE contact_id = $session_contact_id AND contact_client_id = $session_client_id"); @@ -460,10 +466,17 @@ if (isset($_GET['export_statement_pdf'])) { */ $client_id = $session_client_id; - $sql = mysqli_query($mysqli, "SELECT client_name FROM clients WHERE client_id = $client_id LIMIT 1"); + $sql = mysqli_query($mysqli, "SELECT client_currency_code, client_name FROM clients WHERE client_id = $client_id LIMIT 1"); $row = mysqli_fetch_assoc($sql); $client_name = escapeHtml($row['client_name']); + // Match client/statement.php, the guest invoice view and the emailed + // statement - all four render in the client's own currency + $statement_currency_code = escapeHtml($row['client_currency_code']); + if (empty($statement_currency_code)) { + $statement_currency_code = $session_company_currency; + } + $sql = mysqli_query($mysqli, "SELECT company_address, company_city, company_country, company_logo, company_name, company_phone, company_phone_country_code, company_state, company_website, company_zip FROM companies WHERE company_id = 1"); @@ -581,15 +594,15 @@ if (isset($_GET['export_statement_pdf'])) { ' . $invoice_scope . ' ' . $invoice_date . ' ' . $invoice_due . ' - ' . numfmt_format_currency($currency_format, $invoice_amount, $session_company_currency) . ' - ' . numfmt_format_currency($currency_format, $amount_paid, $session_company_currency) . ' - ' . numfmt_format_currency($currency_format, $invoice_balance, $session_company_currency) . ' + ' . numfmt_format_currency($currency_format, $invoice_amount, $statement_currency_code) . ' + ' . numfmt_format_currency($currency_format, $amount_paid, $statement_currency_code) . ' + ' . numfmt_format_currency($currency_format, $invoice_balance, $statement_currency_code) . ' '; } $html .= ' Total Balance Due - ' . numfmt_format_currency($currency_format, $statement_total, $session_company_currency) . ' + ' . numfmt_format_currency($currency_format, $statement_total, $statement_currency_code) . ' '; @@ -621,6 +634,13 @@ if (isset($_POST['edit_profile'])) { $new_password = $_POST['new_password']; + // Without this a hijacked session could set a new password without knowing + // the old one, locking the real contact out of their own portal. + if (!empty($new_password) && !portalReauthenticate($_POST['current_password'] ?? '')) { + flashAlert("That password was not right - your password has not been changed", 'error'); + redirect('profile.php'); + } + if (!empty($new_password)) { $password_hash = password_hash($new_password, PASSWORD_DEFAULT); mysqli_query($mysqli, "UPDATE users SET user_password = '$password_hash' WHERE user_id = $session_user_id"); @@ -913,7 +933,7 @@ if (isset($_GET['add_payment_by_provider'])) { // Notify/log appNotify("Invoice Paid", "Invoice $invoice_prefix$invoice_number automatically paid", "/agent/invoice.php?invoice_id=$invoice_id", $client_id); - logAudit("Invoice", "Payment", "$session_name initiated Stripe payment amount of " . numfmt_format_currency($currency_format, $invoice_amount, $invoice_currency_code) . " added to invoice $invoice_prefix$invoice_number - $pi_id $extended_log_desc", $client_id, $invoice_id); + logAudit("Invoice", "Payment", "$session_contact_name initiated Stripe payment amount of " . numfmt_format_currency($currency_format, $invoice_amount, $invoice_currency_code) . " added to invoice $invoice_prefix$invoice_number - $pi_id $extended_log_desc", $client_id, $invoice_id); triggerCustomAction('invoice_pay', $invoice_id); flashAlert("The amount " . numfmt_format_currency($currency_format, $invoice_amount, $invoice_currency_code) . " paid Invoice $invoice_prefix$invoice_number"); @@ -1374,14 +1394,14 @@ if (isset($_POST['set_recurring_payment'])) { // Get Payment ID for reference $recurring_payment_id = mysqli_insert_id($mysqli); - logAudit("Recurring Invoice", "Auto Payment", "$session_name created Auto Pay for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number in the amount of " . numfmt_format_currency($currency_format, $recurring_invoice_amount, $recurring_invoice_currency_code), $session_client_id, $recurring_invoice_id); + logAudit("Recurring Invoice", "Auto Payment", "$session_contact_name created Auto Pay for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number in the amount of " . numfmt_format_currency($currency_format, $recurring_invoice_amount, $recurring_invoice_currency_code), $session_client_id, $recurring_invoice_id); flashAlert("Automatic Payment $saved_payment_description enabled for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number"); } else { // Delete mysqli_query($mysqli, "DELETE FROM recurring_payments WHERE recurring_payment_recurring_invoice_id = $recurring_invoice_id"); - logAudit("Recurring Invoice", "Auto Payment", "$session_name removed Auto Pay for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number in the amount of " . numfmt_format_currency($currency_format, $recurring_invoice_amount, $recurring_invoice_currency_code), $session_client_id, $recurring_invoice_id); + logAudit("Recurring Invoice", "Auto Payment", "$session_contact_name removed Auto Pay for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number in the amount of " . numfmt_format_currency($currency_format, $recurring_invoice_amount, $recurring_invoice_currency_code), $session_client_id, $recurring_invoice_id); flashAlert("Automatic Payment Disabled for Recurring Invoice $recurring_invoice_prefix$recurring_invoice_number"); } diff --git a/client/profile.php b/client/profile.php index dbc613adf..937eca892 100644 --- a/client/profile.php +++ b/client/profile.php @@ -439,6 +439,17 @@ $sql_actions = mysqli_query( will remember but that is not easy to guess.

+ +
+ +
+ + +
+
+ +
@@ -476,6 +487,15 @@ $sql_actions = mysqli_query(
+
+ +
+ + +
+
+
diff --git a/client/statement.php b/client/statement.php index d3641a255..e45f4fe58 100644 --- a/client/statement.php +++ b/client/statement.php @@ -17,6 +17,18 @@ require_once "includes/inc_all.php"; enforceContactCan('accounting'); +/* + * Amounts render in the client's own currency, matching the guest invoice view + * and the statement an agent emails. This page and its PDF used to render in + * the company currency, so the same invoice carried a different symbol + * depending on where you read it. + */ +$row = mysqli_fetch_assoc(mysqli_query($mysqli, "SELECT client_currency_code FROM clients WHERE client_id = $session_client_id LIMIT 1")); +$statement_currency_code = escapeHtml($row['client_currency_code']); +if (empty($statement_currency_code)) { + $statement_currency_code = $session_company_currency; +} + /* * Payments are summed in a derived table rather than joined directly, or an * invoice with two payments against it would be counted twice. @@ -119,9 +131,9 @@ $statement_total = 0; - - - + + + Total Balance Due - + diff --git a/db.sql b/db.sql index 98160faac..13dd3dfb5 100644 --- a/db.sql +++ b/db.sql @@ -1513,7 +1513,8 @@ CREATE TABLE `logs` ( `log_user_id` int(11) NOT NULL DEFAULT 0, `log_entity_id` int(11) NOT NULL DEFAULT 0, PRIMARY KEY (`log_id`), - KEY `log_created_at` (`log_created_at`) + KEY `log_created_at` (`log_created_at`), + KEY `log_user_id` (`log_user_id`,`log_client_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; /*!40101 SET character_set_client = @saved_cs_client */; diff --git a/js/confirm_modal.js b/js/confirm_modal.js index 718df23b4..42179c957 100644 --- a/js/confirm_modal.js +++ b/js/confirm_modal.js @@ -1,8 +1,13 @@ /** * Confirmation dialogs, on SweetAlert2. * - * Delegated on document rather than bound to the links present at page load, so - * that confirm-link also works on markup injected by an ajax modal. + * Delegated on document rather than bound to the elements present at page load, + * so that confirm-link also works on markup injected by an ajax modal. + * + * Works on an (navigates on confirm) and on a + *