Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V0.1.5 #866

Closed
wants to merge 10 commits into from
Closed

V0.1.5 #866

wants to merge 10 commits into from

Conversation

o-psi
Copy link
Contributor

@o-psi o-psi commented Jan 17, 2024

Small changes for unbilled tickets report, and clean up for passing stripe fees.

@wrongecho
Copy link
Collaborator

Test these changes at: https://v015866.pr-review.itflow.org
(automatic message)

1 similar comment
@wrongecho
Copy link
Collaborator

Test these changes at: https://v015866.pr-review.itflow.org
(automatic message)

functions.php Outdated Show resolved Hide resolved
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@johnnyq johnnyq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-psi The SQL escaping should be performed before passing it to the function so continue to leave the function as a strval and we will need to go through the POST code to make sure SQL is being escaped before passing it off. I think there are still many instances where this will need cleaned up.

@johnnyq
Copy link
Collaborator

johnnyq commented Jan 19, 2024

@o-psi Today Im going to go through the mails codes and make sure all the mail vars are escaped before it passes to the mail function. therefore the mail function should not escape passed vars, as this can lead to double escaping which can lead to further security issues and data corruption. ill update this thread when completed and tested

@johnnyq
Copy link
Collaborator

johnnyq commented Jan 21, 2024

@o-psi, I've completed reviewing the entire mail code. It's important to ensure that POST variables are properly sanitized. Notably, the Body and Subject are no longer sanitized before being sent to the mail function, as the POST variables that are included in the Body and Subject are already sanitized.

However, there are still some scenarios where you may need to manually escape certain characters in the body or subject, especially if you encounter a single quotation mark ('), which should be escaped as '

This should resolve any issues related to mixed variables that are unsanitized and sanitized, making it much easier to diagnose and read the mail queue.

@@ -935,9 +935,13 @@ function addToMailQueue($mysqli, $data) {
$recipient = strval($email['recipient']);
$recipient_name = strval($email['recipient_name']);
$subject = strval($email['subject']);
$body = strval($email['body']);
$body = mysqli_real_escape_string($mysqli,strval($email['body']));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-psi leave this as strval

Comment on lines +941 to +944
$mysqli,
"INSERT INTO email_queue (email_recipient, email_recipient_name, email_from, email_from_name, email_subject, email_content)
VALUES ('$recipient', '$recipient_name', '$from', '$from_name', '$subject', '$body')"
);
Copy link
Collaborator

@johnnyq johnnyq Jan 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-psi Put this back the way it was its easier to read by using set var = '$var_name' and its standardized everywhere in the code.

Comment on lines +345 to +349
$base_url = nullable_htmlentities($config_base_url);

if (!empty($config_smtp_host)) {
$subject = "Payment Received - Invoice $invoice_prefix$invoice_number";
$body = "Hello $contact_name,<br><br>We have received your payment in the amount of " . $pi_currency . $pi_amount_paid . " for invoice <a href='https://$config_base_url/guest_view_invoice.php?invoice_id=$invoice_id&url_key=$invoice_url_key'>$invoice_prefix$invoice_number</a>. Please keep this email as a receipt for your records.<br><br>Amount: " . numfmt_format_currency($currency_format, $pi_amount_paid, $invoice_currency_code) . "<br>Balance: " . numfmt_format_currency($currency_format, '0', $invoice_currency_code) . "<br><br>Thank you for your business!<br><br><br>~<br>$company_name<br>Billing Department<br>$config_invoice_from_email<br>$company_phone";
$body = "Hello $contact_name,<br><br>We have received your payment in the amount of " . $pi_currency . $pi_amount_paid . " for invoice <a href='https://$base_url/guest_view_invoice.php?invoice_id=$invoice_id&url_key=$invoice_url_key'>$invoice_prefix$invoice_number</a>. Please keep this email as a receipt for your records.<br><br>Amount: " . numfmt_format_currency($currency_format, $pi_amount_paid, $invoice_currency_code) . "<br>Balance: " . numfmt_format_currency($currency_format, '0', $invoice_currency_code) . "<br><br>Thank you for your business!<br><br><br>~<br>$company_name<br>Billing Department<br>$config_invoice_from_email<br>$company_phone";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o-psi I just fixed this code a little different way but it fits the rest of the pattern so these changes should not be merged

johnnyq added a commit that referenced this pull request Jan 28, 2024
…counter per Pull Request #866 also added stripe flat and percent config vars to guest view invoice. Note We do not need to merge Pull #866 as all changes nessessary have been implented manually
johnnyq added a commit that referenced this pull request Jan 28, 2024
@johnnyq
Copy link
Collaborator

johnnyq commented Jan 28, 2024

We implemented some of these changes manually and did some things a little differently to make use of the configurable stripe fees and stripe account expense if client pays is off. This can be closed

@johnnyq johnnyq closed this Jan 28, 2024
johnnyq added a commit that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants