search

Star2Billing / a2billing

A2Billing is a telecom switch and billing system capable of providing and billing a range of telecom products and services to customers such as calling card products, residential and wholesale VoIP termination, DID resale and callback services.
www.asterisk2billing.org
Other
181 stars 174 forks source link

checkout_process bug in logic, amount add to card several times #73

Open fureszpeter opened 10 years ago

fureszpeter commented 10 years ago

When PayPal IPN send IPN message, customer balance added every IPN call, each time when IPN notification arrived.

The bug at line 82

//Update the Transaction Status to 1 $QUERY = "UPDATE cc_epayment_log SET status = 2 WHERE id = ".$transactionID;

In the comment, developer want to set the status to 1, but set to 2. When the earlier IPN SET the transaction status to 1, line 82 in the next IPN call SET BACK to status to 2 without any condition checking.

Bug is reproducable

areski commented 10 years ago

I dont understand what fix you suggest?

fureszpeter commented 10 years ago

Ok, I have found more things, and try to collect.

1.--------------ORIGINAL-------------- In the original code, we select the transaction where the status is "new" OR "in process". Even if we found or not any row, in the next step the code update the status of the transaction to "in process", even if it is already processed. So I suggest to put the update section after the IF condition.

// Status - New 0 ; Proceed 1 ; In Process 2 $QUERY = "SELECT id, cardid, amount, vat, paymentmethod, cc_owner, cc_number, cc_expires, creationdate, status, cvv, credit_card_type, currency, item_id, item_type " . " FROM cc_epayment_log " . " WHERE id = ".$transactionID." AND (status = 0 OR (status = 2 AND $NOW_2MIN))"; $transaction_data = $paymentTable->SQLExec ($DBHandle_max, $QUERY);

$item_id = $transaction_data[0][13]; $amount = $transaction_data[0][2]; $item_type = $transaction_data[0][14];

//Update the Transaction Status to 1 $QUERY = "UPDATE cc_epayment_log SET status = 2 WHERE id = ".$transactionID; write_log(LOGFILE_EPAYMENT, basename(FILE).' line:'.LINE."- QUERY = $QUERY"); $paymentTable->SQLExec ($DBHandle_max, $QUERY);

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE). ' line:'.LINE."- transactionID=$transactionID"." ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =".$transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE). ' line:'.LINE."- transactionID=$transactionID"." EPAYMENT RESPONSE: TRANSACTIONID = ".$transactionID. " FROM ".$transaction_data[0][4]."; FOR CUSTOMER ID ".$transaction_data[0][1]."; OF AMOUNT ".$transaction_data[0][2]); }

  1. Update after the if condition____

So the code will looks like this

// Status - New 0 ; Proceed 1 ; In Process 2 $QUERY = "SELECT id, cardid, amount, vat, paymentmethod, cc_owner, cc_number, cc_expires, creationdate, status, cvv, credit_card_type, currency, item_id, item_type " . " FROM cc_epayment_log " . " WHERE id = " . $transactionID . " AND (status = 0 OR (status = 2 AND $NOW_2MIN))"; $transaction_data = $paymentTable->SQLExec($DBHandle_max, $QUERY);

$item_id = $transaction_data[0][13]; $amount = $transaction_data[0][2]; $item_type = $transaction_data[0][14];

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

//Update the Transaction Status to 1 $QUERY = "UPDATE cc_epayment_log SET status = 2 WHERE id = " . $transactionID; write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- QUERY = $QUERY"); $paymentTable->SQLExec($DBHandle_max, $QUERY);

  1. -----------------WRONG IF CONDITION-------------------- The following condition check is not working as expected. We exit if the result is not an array AND the array count is 0. It will never TRUE! If it is not an array, the count will never 0. . We want to exit, but the if condition will go to the else section, because is_array($transaction_data) is TRUE.

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

  1. _____ MODIFIED IF CONDITION__ We want to exit, if the result is not an array, or it is an array BUT it is empty

if (!is_array($transaction_data) || (is_array($transaction_data) && count($transaction_data)) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

  1. _____ Long-Long script execution time, is the source of all bug

    I realized from the checkout log, that the checkout_process script execution time takes 3-4 minutes. This is the source of the duplication bug. For example the first thread start the execution at 14:00:00 and stops at 14:03:00. Read the data from the database, the transaction status, etc... The second thread start the execution at 14:01:00 and ending at 14:04:00 . Because the first thread is still running, and it is still not updated the status of the transaction to 1, the second thread read the status and think that it is in "in process" status. When thread 1 finishing, creating the invoice, add the amount to the user, and set the status to "proceed". The second thread do it again, create invoice, add the amount to the card and so on. Each thread means duplication.

ORIGINAL CODE:

// Headers PayPal system to validate $header .= "POST /cgi-bin/webscr HTTP/1.1\r\n"; $header .= "Content-Type: application/x-www-form-urlencoded\r\n"; $header .= "Host: www.paypal.com\r\n"; $header .= "Content-Length: " . strlen($req) . "\r\n\r\n"; for ($i = 1; $i <= 3; $i++) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-OPENDING HTTP CONNECTION TO " . PAYPAL_VERIFY_URL); $fp = fsockopen(PAYPAL_VERIFY_URL, 443, $errno, $errstr, 30); if ($fp) { break; } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . " -Try#" . $i . " Failed to open HTTP Connection : " . $errstr . ". Error Code: " . $errno); sleep(3); } } if (!$fp) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-Failed to open HTTP Connection: " . $errstr . ". Error Code: " . $errno); exit(); } else { fputs($fp, $header . $req); $flag_ver = False; while (!feof($fp)) { $res = fgets($fp, 1024); $gather_res .= $res; $res = trim($res);

MODIFIED CODE:

In the VERIFIED condition check, just put the break; . After that the long execution time gone.

// Headers PayPal system to validate $header .= "POST /cgi-bin/webscr HTTP/1.1\r\n"; $header .= "Content-Type: application/x-www-form-urlencoded\r\n"; $header .= "Host: www.paypal.com\r\n"; $header .= "Content-Length: " . strlen($req) . "\r\n\r\n"; for ($i = 1; $i <= 3; $i++) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-OPENDING HTTP CONNECTION TO " . PAYPAL_VERIFY_URL); $fp = fsockopen(PAYPAL_VERIFY_URL, 443, $errno, $errstr, 30); if ($fp) { break; } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . " -Try#" . $i . " Failed to open HTTP Connection : " . $errstr . ". Error Code: " . $errno); sleep(3); } } if (!$fp) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-Failed to open HTTP Connection: " . $errstr . ". Error Code: " . $errno); exit(); } else { fputs($fp, $header . $req); $flag_ver = False; while (!feof($fp)) { $res = fgets($fp, 1024); $gather_res .= $res; $res = trim($res); if (strcmp($res, "VERIFIED") == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Verified "); $flag_ver = True; * break;* } } if (!$flag_ver) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Failed \nreq=$req\n$gather_res"); $security_verify = false; } } fclose($fp);

  1. TRANSACTION

Finally I put the whole code in a transaction, so something went wrong, just ROLLBACK everything.

2014/1/7 Areski Belaid notifications@github.com

I dont understand what fix you suggest?

— Reply to this email directly or view it on GitHubhttps://github.com/Star2Billing/a2billing/issues/73#issuecomment-31730887 .

areski commented 10 years ago

Could you send a patch so we can see the diff? The best way might actually to fork a2billing on github, apply your changes and send a pull request. https://help.github.com/articles/creating-a-pull-request

On Tue, Jan 7, 2014 at 5:42 PM, fureszpeter notifications@github.comwrote:

Ok, I have found more things, and try to collect.

1.--------------ORIGINAL-------------- In the original code, we select the transaction where the status is "new" OR "in process". Even if we found or not any row, in the next step the code update the status of the transaction to "in process", even if it is already processed. So I suggest to put the update section after the IF condition.

// Status - New 0 ; Proceed 1 ; In Process 2 $QUERY = "SELECT id, cardid, amount, vat, paymentmethod, cc_owner, cc_number, cc_expires, creationdate, status, cvv, credit_card_type, currency, item_id, item_type " . " FROM cc_epayment_log " . " WHERE id = ".$transactionID." AND (status = 0 OR (status = 2 AND $NOW_2MIN))"; $transaction_data = $paymentTable->SQLExec ($DBHandle_max, $QUERY);

$item_id = $transaction_data[0][13]; $amount = $transaction_data[0][2]; $item_type = $transaction_data[0][14];

//Update the Transaction Status to 1 $QUERY = "UPDATE cc_epayment_log SET status = 2 WHERE id = ".$transactionID;

write_log(LOGFILE_EPAYMENT, basename(FILE).' line:'.LINE."- QUERY

$QUERY"); $paymentTable->SQLExec ($DBHandle_max, $QUERY);

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE). ' line:'.LINE."- transactionID=$transactionID"." ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =".$transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE). ' line:'.LINE."- transactionID=$transactionID"." EPAYMENT RESPONSE: TRANSACTIONID = ".$transactionID. " FROM ".$transaction_data[0][4]."; FOR CUSTOMER ID ".$transaction_data[0][1]."; OF AMOUNT ".$transaction_data[0][2]); }

  1. Update after the if condition____

So the code will looks like this

// Status - New 0 ; Proceed 1 ; In Process 2 $QUERY = "SELECT id, cardid, amount, vat, paymentmethod, cc_owner, cc_number, cc_expires, creationdate, status, cvv, credit_card_type, currency, item_id, item_type " . " FROM cc_epayment_log " . " WHERE id = " . $transactionID . " AND (status = 0 OR (status = 2 AND $NOW_2MIN))"; $transaction_data = $paymentTable->SQLExec($DBHandle_max, $QUERY);

$item_id = $transaction_data[0][13]; $amount = $transaction_data[0][2]; $item_type = $transaction_data[0][14];

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

//Update the Transaction Status to 1 $QUERY = "UPDATE cc_epayment_log SET status = 2 WHERE id = " . $transactionID; write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- QUERY = $QUERY"); $paymentTable->SQLExec($DBHandle_max, $QUERY);

  1. -----------------WRONG IF CONDITION-------------------- The following condition check is not working as expected. We exit if the result is not an array AND the array count is 0. It will never TRUE! If it is not an array, the count will never 0. . We want to exit, but the if condition will go to the else section, because is_array($transaction_data) is TRUE.

if (!is_array($transaction_data) && count($transaction_data) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

  1. _____ MODIFIED IF CONDITION__ We want to exit, if the result is not an array, or it is an array BUT it is empty

if (!is_array($transaction_data) || (is_array($transaction_data) && count($transaction_data)) == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " ERROR INVALID TRANSACTION ID PROVIDED, TRANSACTION ID =" . $transactionID); exit(); } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "- transactionID=$transactionID" . " EPAYMENT RESPONSE: TRANSACTIONID = " . $transactionID . " FROM " . $transaction_data[0][4] . "; FOR CUSTOMER ID " . $transaction_data[0][1] . "; OF AMOUNT " . $transaction_data[0][2]); }

  1. _____ Long-Long script execution time, is the source of all bug

    I realized from the checkout log, that the checkout_process script execution time takes 3-4 minutes. This is the source of the duplication bug. For example the first thread start the execution at 14:00:00 and stops at 14:03:00. Read the data from the database, the transaction status, etc... The second thread start the execution at 14:01:00 and ending at 14:04:00 . Because the first thread is still running, and it is still not updated the status of the transaction to 1, the second thread read the status and think that it is in "in process" status. When thread 1 finishing, creating the invoice, add the amount to the user, and set the status to "proceed". The second thread do it again, create invoice, add the amount to the card and so on. Each thread means duplication.

ORIGINAL CODE:

// Headers PayPal system to validate $header .= "POST /cgi-bin/webscr HTTP/1.1\r\n"; $header .= "Content-Type: application/x-www-form-urlencoded\r\n"; $header .= "Host: www.paypal.com\r\n"; $header .= "Content-Length: " . strlen($req) . "\r\n\r\n"; for ($i = 1; $i <= 3; $i++) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-OPENDING HTTP CONNECTION TO " . PAYPAL_VERIFY_URL); $fp = fsockopen(PAYPAL_VERIFY_URL, 443, $errno, $errstr, 30); if ($fp) { break; } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . " -Try#" . $i . " Failed to open HTTP Connection : " . $errstr . ". Error Code: " . $errno); sleep(3); } } if (!$fp) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-Failed to open HTTP Connection: " . $errstr . ". Error Code: " . $errno); exit(); } else { fputs($fp, $header . $req); $flag_ver = False; while (!feof($fp)) { $res = fgets($fp, 1024); $gather_res .= $res; $res = trim($res);

  • if (strcmp($res, "VERIFIED") == 0) {*
  • write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Verified ");*
  • $flag_ver = True;*
  • }* } if (!$flag_ver) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Failed \nreq=$req\n$gather_res"); $security_verify = false; } } fclose($fp);

MODIFIED CODE:

In the VERIFIED condition check, just put the break; . After that the long execution time gone.

// Headers PayPal system to validate $header .= "POST /cgi-bin/webscr HTTP/1.1\r\n"; $header .= "Content-Type: application/x-www-form-urlencoded\r\n"; $header .= "Host: www.paypal.com\r\n"; $header .= "Content-Length: " . strlen($req) . "\r\n\r\n"; for ($i = 1; $i <= 3; $i++) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-OPENDING HTTP CONNECTION TO " . PAYPAL_VERIFY_URL); $fp = fsockopen(PAYPAL_VERIFY_URL, 443, $errno, $errstr, 30); if ($fp) { break; } else { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . " -Try#" . $i . " Failed to open HTTP Connection : " . $errstr . ". Error Code: " . $errno); sleep(3); } } if (!$fp) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-Failed to open HTTP Connection: " . $errstr . ". Error Code: " . $errno); exit(); } else { fputs($fp, $header . $req); $flag_ver = False; while (!feof($fp)) { $res = fgets($fp, 1024); $gather_res .= $res; $res = trim($res); if (strcmp($res, "VERIFIED") == 0) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Verified "); $flag_ver = True;

  • break;* } } if (!$flag_ver) { write_log(LOGFILE_EPAYMENT, basename(FILE) . ' line:' . LINE . "-PAYPAL Transaction Verification Status: Failed \nreq=$req\n$gather_res"); $security_verify = false; } } fclose($fp);
    1. TRANSACTION

Finally I put the whole code in a transaction, so something went wrong, just ROLLBACK everything.

2014/1/7 Areski Belaid notifications@github.com

I dont understand what fix you suggest?

— Reply to this email directly or view it on GitHub< https://github.com/Star2Billing/a2billing/issues/73#issuecomment-31730887>

.

— Reply to this email directly or view it on GitHubhttps://github.com/Star2Billing/a2billing/issues/73#issuecomment-31754181 .

Kind regards, /Areski

Arezqui Belaid, areski@gmail.com Founder at Star2Billing (www.star2billing.com)

Tel: +34650784355 Twitter: http://twitter.com/areskib LinkedIn: http://www.linkedin.com/in/areski

rabongithub commented 1 year ago

Hello, did you get it fixed ? is it possible to get the whole working code. thanks.