tpay-com / tpay-php

MIT License
24 stars 27 forks source link

tpay\PaymentBasic::checkPayment() is not working correctly #5

Closed macieklamberski closed 7 years ago

macieklamberski commented 7 years ago

While integrating payments into my application, I noticed that all notifications sent back from tpay.com to URL in my application (provided in wyn_url parameter while creating transaction) are treated as invalid. The error message I was getting was this:

Field "tr_status" has unsupperted value.

After some digging into code I found that the problem is related to validation of the request from tpay.com. Here's exact line where the problematic part starts: /src/_class_tpay/validate.php:1079.

When you dump value of the $ready that is iterated in the loop, you'll see something similar to this:

array:11 [
  "tr_id" => ""
  "tr_date" => ""
  "tr_crc" => ""
  "tr_amount" => 0.0
  "tr_paid" => 0.0
  "tr_desc" => ""
  "tr_status" => ""
  "tr_error" => ""
  "tr_email" => ""
  "md5sum" => ""
  "test_mode" => 0
]

which is completely different from the original data sent in request from tpay.com:

array:12 [
  "id" => "27496"
  "tr_id" => "TR-SH2-EK57YX"
  "tr_date" => "2017-01-22 20:26:39"
  "tr_crc" => "1"
  "tr_amount" => "1643.00"
  "tr_paid" => "1643.00"
  "tr_desc" => "Opłata za czynsz"
  "tr_status" => "TRUE"
  "tr_error" => "none"
  "tr_email" => "kowalska.dominik@example.net"
  "test_mode" => "1"
  "md5sum" => "ff09062fe98d8874306de48f0a7e13a9"
]

It's no suprise that validation fails. After digging deeper, I noticed that this $ready variable is combined from values generated in /src/_class_tpay/validate.php:1056 using Util::post method. I bet the problem with values replaced with zeros or empty strings is laying somewhere within the body of this method, so I'd start looking there.

piotrjozwiak commented 7 years ago

Sending additional rows in POST array will not make validatioan fail. I have tested THIS function with your data and it works fine. The function returns valid parameres.

Maybe the problem is somewhere else, could you send me your response validation code?

macieklamberski commented 7 years ago

I do basic checking if the request is valid by returning false if checkPayment throws an exception.

/**
 * Validate request sent from payment gateway that is used to update
 * order status in local database.
 *
 * @param  Request  $request
 * @return bool
 */
public function valid(Request $request)
{
    try {
        (new tpay\PaymentBasic(
            self::$config['merchantId'],
            self::$config['merchantSecret']
        ))->checkPayment();
    } catch (tpay\TException $exception) {
        return false;
    }

    return true;
}

Dumping $exception->getMessage(), I get below, even after updating the library to the newest version (from today).

Field "tr_status" has unsupperted value in file /.../vendor/tpaycom/tpay/src/_class_tpay/validate.php line: 1318 in /.../vendor/tpaycom/tpay/src/_class_tpay/validate.php:1318

macieklamberski commented 7 years ago

To help you with debugging, here's complete dump of $_POST that I'm receiving from tpay.com.

[
  'id' => '27496',
  'tr_id' => 'TR-SH2-EK57YX',
  'tr_date' => '2017-01-22 20:26:39',
  'tr_crc' => '1',
  'tr_amount' => '1643.00',
  'tr_paid' => '1643.00',
  'tr_desc' => '...',
  'tr_status' => 'TRUE',
  'tr_error' => 'none',
  'tr_email' => 'kowalski.dominik@example.net',
  'test_mode' => '1',
  'md5sum' => 'ff09062fe98d8874306de48f0a7e13a9',
]
piotrjozwiak commented 7 years ago

So this exception is thrown at this line. The validateOptions function is always called by getResponse function which I have already tested using your data provided before. When I test notifications sent from our server, the "var_dumped" data looks like this

array(11) { ["id"]=> string(5) "25223" ["tr_id"]=> string(12) "TR-C4Y-TST3X" ["tr_date"]=> string(19) "2017-01-24 15:36:25" ["tr_crc"]=> string(6) "sasass" ["tr_amount"]=> string(5) "10.00" ["tr_paid"]=> string(5) "10.00" ["tr_desc"]=> string(4) "test" ["tr_status"]=> string(4) "TRUE" ["tr_error"]=> string(4) "none" ["tr_email"]=> string(0) "" ["md5sum"]=> string(32) "5c477f8175e68fe7ba113a88c6a08e56" }

So the data provided by our server are always string type and tr_status parameter has valid value (TRUE string) defined here

Here is my test function

public function valid()

{ $_POST = array( 'id' => '27496', 'tr_id' => 'TR-SH2-EK57YX', 'tr_date' => '2017-01-22 20:26:39', 'tr_crc' => '1', 'tr_amount' => '1643.00', 'tr_paid' => '1643.00', 'tr_desc' => 'Opłata za czynsz', 'tr_status' => 'TRUE', 'tr_error' => 'none', 'tr_email' => 'kowalska.dominik@example.net', 'test_mode' => '1', 'md5sum' => 'ff09062fe98d8874306de48f0a7e13a9' ); try { (new PaymentBasic( 1010, 'merchantSecret' ))->checkPayment(); } catch (TException $exception) { echo 'Caught exception: ', $exception->getMessage(), "\n"; return false;

    }

    return true;
}

To test it offline you should change this code (lines 144-147)

to this one

    if (!isset($_POST[$name])) {

return false; } $val = $_POST[$name];

macieklamberski commented 7 years ago

Using method you described I get exactly the same result as with real POST call from tpay:

Field "tr_status" has unsupported value in file /.../vendor/tpaycom/tpay/src/_class_tpay/validate.php line: 1318"

I'll say it again—I'm using latest version of the library.


So the data provided by our server are always string type and tr_status parameter has valid value (TRUE string) defined here

Have you tried it yourself? Or you're saying that just by analyzing code of the library? Does the above test work for you?

piotrjozwiak commented 7 years ago

Yes, I have tested that and the code works for me.

macieklamberski commented 7 years ago

To prove that this bug exists, I prepared demonstration here: https://github.com/lamberski/tpay-error. This repository consists of:

Just download it and run index.php and you'll see that valid request is treated as invalid by the library.

piotrjozwiak commented 7 years ago

I told you, you have to change lines 144-147

from:

if (!filter_input(INPUT_POST, $name)) { return false; } $val = filter_input(INPUT_POST, $name);

to:

if (!isset($_POST[$name])) { return false; } $val = $_POST[$name];

and you have changed to:

    if (!isset($_POST[$name])) {
        return false;
    }
    $val = filter_input(INPUT_POST, $name);

So it can not work. Do as I said and see it working.

macieklamberski commented 7 years ago

I just don't understand why you cannot apply the fix directly to files in this repository and ask me to modify the library myself.

macieklamberski commented 7 years ago

This is just unbelievable that you don't want to fix it and ask developers to modify the code themselves.

As I stated first, this error also occurs with real calls from tpay, so it's not only matter of adjusting the code of Util class to work with mocked $_POST.

Have you tested the library it with real calls too? Does it work without any modifications?

Again: If such modification in Util class is needed to fix this bug, why can't you apply this change directly in code of the library?

piotrjozwiak commented 7 years ago

Yes I have tested it live and it works well. I will not merge noncompilant code to our libs - accessing $_POST directly is noncompilant solution. I have made some server IP verification changes due to INPUT_SERVER php bugs in filter_input Check if everything is working now.

macieklamberski commented 7 years ago

YES! Thank you!