[D8] Commerce EuPlatesc Payment Gateway

Hi,

First thing I did was running phpcs on your project and there are quite some things to be re-worked. I would reccomend you to fix those issues first before I (or anyone else) proceeds with the review.

Here is a list of the issues identified.

❯ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml --ignore=node_modules,bower_components,vendor commerce_euplatesc

FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/README.md
----------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------
 1 | WARNING | File has mixed line endings; this may cause incorrect results
----------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/commerce_euplatesc.module
------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AND 1 WARNING AFFECTING 4 LINES
------------------------------------------------------------------------------------------------------------------------------------
  1 | ERROR   | [x] The PHP open tag must be followed by exactly one blank line
  1 | ERROR   | [x] Missing file doc comment
 19 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 12
 21 | ERROR   | [x] Expected one space after the comma, 0 found
 22 | WARNING | [x] A comma should follow the last multiline array item. Found: 'https://www.drupal.org/project/commerce_euplatesc'
------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/src/PluginForm/EuPlatescCheckoutForm.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-----------------------------------------------------------------------------------------------------------------------
  8 | ERROR | [x] Missing class doc comment
 33 | ERROR | [x] Expected 1 blank line after function; 0 found
 34 | ERROR | [x] The closing brace for the class must have an empty line before it
-----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/src/Plugin/Commerce/PaymentGateway/EuPlatescCheckout.php
---------------------------------------------------------------------------------------------------------------------------------------
FOUND 40 ERRORS AND 5 WARNINGS AFFECTING 45 LINES
---------------------------------------------------------------------------------------------------------------------------------------
  23 | ERROR   | [x] There must be one blank line after the last USE statement; 3 found;
  26 | ERROR   | [ ] More than 2 empty lines are not allowed
 179 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 205 | ERROR   | [x] Concat operator must be surrounded by a single space
 244 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 254 | ERROR   | [x] Parameter comment indentation must be 3 spaces, found 2 spaces
 261 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 262 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 263 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 265 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 266 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 268 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 270 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 272 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 273 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 274 | ERROR   | [x] Array indentation error, expected 6 spaces but found 7
 275 | ERROR   | [x] Array closing indentation error, expected 4 spaces but found 5
 279 | ERROR   | [x] Expected 3 space(s) before asterisk; 4 found
 280 | ERROR   | [x] Expected 3 space(s) before asterisk; 4 found
 307 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 317 | ERROR   | [ ] Parameter $payment_state is not described in comment
 321 | ERROR   | [x] Parameter comment indentation must be 3 spaces, found 2 spaces
 323 | ERROR   | [x] Parameter comment indentation must be 3 spaces, found 2 spaces
 349 | WARNING | [x] A comma should follow the last multiline array item. Found: )
 359 | WARNING | [ ] Possible useless method overriding detected
 365 | ERROR   | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph
 368 | ERROR   | [x] Parameter comment indentation must be 3 spaces, found 2 spaces
 370 | ERROR   | [x] Parameter comment indentation must be 3 spaces, found 2 spaces
 372 | ERROR   | [x] Expected "string" but found "string." for function return type
 373 | ERROR   | [x] Return comment indentation must be 3 spaces, found 2 spaces
 375 | ERROR   | [ ] Type hint "array" missing for $data
 394 | ERROR   | [x] Line indented incorrectly; expected 2 spaces, found 1
 396 | ERROR   | [ ] Doc comment short description must be on a single line, further text should be a separate paragraph
 403 | ERROR   | [x] Expected "string" but found "string." for function return type
 406 | ERROR   | [ ] Private method name "EuPlatescCheckout::hashSHA1" is not in lowerCamel format
 407 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 408 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 410 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 411 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 4
 412 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 414 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 415 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 416 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 418 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
 419 | ERROR   | [x] Line indented incorrectly; expected 4 spaces, found 3
---------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 38 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/src/Plugin/Commerce/PaymentGateway/EuPlatescCheckoutInterface.php
------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------------------------------------------------------------------------------
 19 | ERROR | [x] Whitespace found at end of line
 30 | ERROR | [x] Whitespace found at end of line
 31 | ERROR | [x] Additional blank lines found at end of doc comment
 32 | ERROR | [x] Expected 1 blank line after function; 0 found
 33 | ERROR | [x] The closing brace for the interface must have an empty line before it
------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/src/Event/EuPlatescEvents.php
------------------------------------------------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 7 LINES
------------------------------------------------------------------------------------------------------------
 11 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 12 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 13 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 14 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 15 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 16 | ERROR | [x] Line indented incorrectly; expected 3 spaces, found 4
 27 | ERROR | [x] Expected 1 newline at end of file; 0 found
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------


FILE: /Users/bram.driesen/DrupalContrib/web/modules/contrib/commerce_euplatesc/src/Event/EuPlatescPaymentEvent.php
------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------
 27 | ERROR | [x] There must be no blank lines after the function comment
 42 | ERROR | [x] Expected 1 newline at end of file; 0 found
 42 | ERROR | [x] The closing brace for the class must have an empty line before it
------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------

Time: 183ms; Memory: 10MB

I did notice you have the following in your info.yml while quickly browsing through your project.

core: 8.x
core_version_requirement: ^8 || ^9

If I'm not mistaken you should not specify the core: 8.x key if the core_version_requirement key is present in your info.yml file.

Ignore the part above about the core key :-).

However you can also specify the drupal requirements in the composer.json.

{
  "require": {
    "drupal/core": "^8.7.7 || ^9"
  }
}

(Example from https://www.drupal.org/node/3070687)

I added the PAreview checklist link.
It reports wrong line endings in some files and wrong indentation in some lines. Without other issues, I would not consider them application blockers.

Hi @Adevms,

Manual Review

Coding style

Issue Listed are as below

1. Please add null or empty check before passing foreach loop $order->getItems() before using in code at line no. 200 [foreach ($order->getItems() as $item) ]in EuPlatescCheckout.php (
2. Please add null or empty check before passing foreach loop $data before using in code at line no. 382 [foreach ($data as $d) {]in EuPlatescCheckout.php (

I don't see what that would be needed. If it's null or empty it simply would be ignored by the foreach loop.

@bramDriesen

The review point shared #7 is not blocker but if $order variable null or empty then $order->getItems() generates the warning calling undefined methods on$order object. So I added my comments.

I highly doubt it is possible to end up in the payment gateway if you don't have an order object. I'm quite sure Drupal Commerce has things in place to catch or prevent that from happening.

Automated Review

PAreview is still giving a few remarks http://pareview.net/r/35 but certainly a lot less as before! :)

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation.

Master Branch

Follows the guidelines for master branch. Although I find it strange only stable releases have been tagged for this module. Meaning there was never an alpha, beta, or release candidate stage.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Follows the guidelines for in-project documentation and/or the README Template. But the formatting is still incorrect, the file is not rendered properly.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

Meets the security requirements.

Coding style & Drupal API usage

1. As mentioned above, the readme.md doesn't render correctly.
2. Maybe for the configuration form fields provide an indication on where you can find the relevant keys & secrets on the EuPlatesc platform. Now the module just states basically the same as the field label.
3. (*) EuPlatescCheckout.php#L138 Why are you storing configuration in a state, and not in the configuration system? This function doesn't seem properly implemented as the commerce documentation is describing https://docs.drupalcommerce.org/commerce2/developer-guide/payments/creat...
4. (+) EuPlatescCheckout.php#L163 == "0" do you need to check for string 0? Why double quotes (module uses single almost everywhere, should be consistent). Preferably always do strict checking ===.
5. EuPlatescCheckout.php#L201 can be rewritten in a single line.
6. (+) PluginForm/EuPlatescCheckoutForm.php#L29 You are casting items into an array $data. But the issue is that if setEuPlatescCheckoutData goes into an error and doesn't return any data to be processed, the $data array will not exist and you'll get a fatal error on the buildRedirectForm because the array doesn't exist. This can be fixed by simply initiating a empty array before processing the foreach.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I accidentally put my review in the issue summary (didn't have my coffee yet ☕😅). I reverted it back and put my actual comment in the comment. Updating to needs work for the * items.

Thanks for you contribution!

manual review:

1. EuPlatescCheckout::onReturn(): do not use the "!==" operator to compare digital signatures or hashes as that is prone to timing attacks. Use hash_equals)( instead, see https://www.php.net/manual/en/function.hash-equals.php
2. EuPlatescCheckout::hashSha1(): function name is wrong since you are not using sha1()? You are using md5() as hashing function there. md5 is an outdated hashing mechanism that has been broken many times and must not be used for digital signatures. Is there a way that you can implement the payment gateway in a secure way that uses a modern hashing function such as sha256?
3. EuPlatescCheckout::setEuPlatescCheckoutData(): in order to create a cryptographically secure nonce you should use Crypt::randomBytesBase64() or similar.

Although the weak hash functions and the timing attack weakness are not easily to exploit security vulnerabilities (at least not as far as I can see here), I think they need to be fixed before we can give security coverage for this module. Is there a way you can work with the payment gateway provider to implement a workflow with modern and secure hash functions and signatures?

I am closing this application since there have been replies in the past 5 months.