qenta-cee / checkout-client-library

Client library used mainly by QENTA Checkout Page and Seamless plugins for payment processing
GNU General Public License v2.0
6 stars 2 forks source link

FrontendClient should throw if malformed customerId, shopId, language or secret are passed #31

Open ThisIsJustARandomGuy opened 2 years ago

ThisIsJustARandomGuy commented 2 years ago

https://github.com/qenta-cee/checkout-client-library/blob/c239fc09eeb2984cf6810a26677ded666cc5acab/src/QentaCEE/QPay/FrontendClient.php#L284

Consider the following example (observe the spaces around the secret):

// init.php
$clientId = "D32123";
$shopId = "...";
$secret = "      VERYSECRET     ";

// The constructor calls trim() on the secret here so it becomes "VERYSECRET"
$client = new FrontendClient([
        'CUSTOMER_ID' => $customerId,
        'SHOP_ID' => $shopId,
        'SECRET' => $secret,
        'LANGUAGE' => 'en',
    ]);

// Set up the client (skipped here)

// This works while it should *not*
$response = $client->initiate();

$response->hasFailed(); // This returns false but it should have failed because the $secret is wrong

// Redirect the customer. Pseudocode
redirect_to($response->getRedirectUrl());

The customer is now able to complete the payment.

// return.php
// The customer has paid and was now redirected back to the shop.
// So we get the request data
$requestData = $_REQUEST;

// And call the return factory
// Here, $secret is *not* modified using trim().
// So it stays "      VERYSECRET     " instead of "VERYSECRET" which was used to initialize the payment
$return = ReturnFactory::getInstance($requestData, $secret);

// Of course now the return cannot be validated:
$return->validate(); // This returns false even though the payment was a success!

So the customer is able to complete the payment, gets redirected back to the shop and then gets an error message.

Either ReturnFactory::getInstance($return, $secret) should also call trim() on the $secret or the FrontendClient should fail to initialize if faulty login data is passed to it (my preferred solution). Currently, the payment is initialized and the user can pay but then the validation back at the shop fails. In my opinion the payment init process should fail instead of silently modifying the provided login data and later failing when the payment is already complete. What's worse is that this applies to handling of both, the normal returns as well as IPNs.