[D9] Commerce Payin-Payout

Thank you for applying! I added the PAreview checklist link. Reviewers will check the project and post comments to list what should be changed.

Hi!

Please, adjust your README to fit best practices.

Also, you have two README files, .txt and .md. Use only one for that.

1. I'm not sure that this is safe. Drupal uses Symfony's Request and Response objects. This one comes from Laminas. I think it is better to replace with Symfony Response and adjust header for specific Content-Type. If you want to use that response, at least, you should require Drupal 8.9.0 at minimum at your .info.yml file, because this is the release where Laminas was introduced as replacement for Zend.
2. This line is related to the previous point. Laminas are introduced in Drupal 8.9.0
3. Since this is a new project, and it targets Drupal 9, you should rename your branch to 1.x or 1.0.x. The old naming is not supported for Drupal 9.
4. It's better to replace this one with $phone_number = $order->getBillingProfile()->get($phone_field_name)->value;. But I think this is my personal preference :)
5. Also, you should wrap this line with condition, to check that field is exists with hasField(). If it doesn't, you should throw \Drupal\commerce_payment\Exception\PaymentGatewayException. That way Drupal Commerce will handle it without WSOD for user.
6. Incorrect formatting. Either split it into multiline for each argument or move array opening into main line.

UPD. Apparently using Laminas Response isn't safe, because of #2979588: Deprecate Laminas\Feed reader and writer services

hi.

Unused property https://git.drupalcode.org/project/commerce_payin_payout/-/blob/1.x/src/...

Nitpick
Syntax error "Check" https://git.drupalcode.org/project/commerce_payin_payout/-/blob/1.x/src/...

I not recommend keep entity storage. https://git.drupalcode.org/project/commerce_payin_payout/-/blob/1.x/src/...
Load entity storage object by a real need

Also, keep in sync requirements of .info.yml file and .composer.json.

Your composer.json require Drupal core "drupal/core": "^8.8 || ^9",, but info file allows to use any Drupal 8 core_version_requirement: ^8 || ^9.

I think it's better to update info.yml to core_version_requirement: ^8.8 || ^9, unless you are sure that your module will work on any Drupal 8 version. If so, you have another problem. For Drupal <8.7.4 you should specify core. So it's better to bump module requirement to ^8.8.

Updated pareview link to appropriate branch, to avoid issue with suggestion of renaming 1.x into 9.x-1.x.

upd. Seems we did the same at the same time ><

I'll check your module more deeply later.

Incorrect PHPDoc for constructor

\Drupal\commerce_payin_payout\PluginForm\PayinPayoutForm::__construct has {@inheritdoc} PHPDoc, but it doesn't inherit anything. You should describe constructor correctly.

Unused variable $key

\Drupal\commerce_payin_payout\Plugin\Commerce\PaymentGateway\PayinPayout::getCustomerFieldOptions is not uses $key variable in for loop. You should remove it.

The $api_url can be undefined

Here \Drupal\commerce_payin_payout\PluginForm\PayinPayoutForm::buildConfigurationForm you have such code:

    // Set API url for selected payment mode.
    switch ($this->plugin->getMode()) {
      case 'test':
        $api_url = self::API_URL_TEST;
        break;

      case 'live':
        $api_url = self::API_URL_LIVE;
        break;
    }

In case if mode will be broken for some reason, you have not provided a fallback solution. It's better to provide default: with test URL or even better, throw PaymentGatewayException to log such problems.

Source link doesn't use HTTPS

In composer.json file, source link (http://cgit.drupalcode.org/commerce_payin_payout) uses HTTP, it's better replace by HTTPS.

Useless variable declaration in \Drupal\commerce_payin_payout\Utility\PayinPayoutHelper::formatCurrency

$currency_formatted = $currency_code; this assignment does nothing, actually. It can be removed. You can overwrite $currency_code directly.

Useless variable $payment_state

In \Drupal\commerce_payin_payout\Plugin\Commerce\PaymentGateway\PayinPayout::onNotify you have declared a variable $payment_state = 'completed'; which is uses far away from where it declared, but also, it won't change in any way. You can replace 'state' => $payment_state, with 'state' => 'completed', and remove variable in that case.

You also can improve this part of your change \Drupal\commerce_payin_payout\Utility\PayinPayoutHelper::formatCurrency:

  public function formatCurrency(string $currency_code) {
    $currencies_map = [
      'RUB' => 'RUR',
      'RUR' => 'RUB',
    ];

    if (array_key_exists($currency_code, $currencies_map)) {
      return $currencies_map[$currency_code];
    }

    return $currency_code;
  }

It can be simplified to:

  public function formatCurrency(string $currency_code) {
    $currencies_map = [
      'RUB' => 'RUR',
      'RUR' => 'RUB',
    ];
    return $currencies_map[$currency_code] ?? $currency_code;
  }

But this is just a small improvement.

All the rest seems fine for me now.

Thanks for you contribution!

manual review:

1. PayinPayoutHelper::generateSign(): It looks like this is creating some kind of security hash, HMAC or similar? You must never use md5() in security-critical contexts, because it is cryptographically broken. Is this a requirement of the payment gateway? Does the payment gateway offer a secure way of validating messages that does not use md5? This is currently a security blocker, I don't think we can give security coverage to projects that use md5 in security contexts.
2. PayinPayout::onNotify(): You are directly comparing "$request_sign !== $validation_sign" which is vulnerable to timing attacks. You need to use hash_equals() instead whenever you compare security-critical hashes. This is also a security blocker right now, but should be easy to fix.

I see, that is unfortunate.

I'll assign this to greggles from the security team, maybe he has time to check here.

@greggles: Hi, can we give security coverage to payment gateway integrations that require to use md5() for validating payments? Do we have any policy on that?

From my quick searches it seems that this has elements of an hmac, but is not an hmac. It also seems that using md5 in an hmac type of system is not necessarily a problem (see this post from 2019).

I think the gateway (and therefore the module) would ideally improve its HMAC strategy and also the hashing function, but that doesn't feel to me like something we should block the module on.

However, it would be great to do two things:
1. Document in a code comment that the md5 is required by the gateway and point to relevant documentation
2. Put that on the project page

Marking as "needs work" for those 2 ideas. They are not blocking requests, but optional ones and good ones from my perspective.

Thanks a lot for the advice here greggles, that helps and I agree!

Reviewed the code once more and it looks good to me.

Thanks for your contribution, Alexander!

I updated your account so you can opt into security advisory coverage now.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.