[D7] Ubercart Veritrans

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

[Best practice issues identified by pareview.sh.]
FILE: /var/www/drupal-7-pareview/pareview_temp/ubercart_veritrans.install
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
5 | ERROR | There must be no blank lines after the function comment
--------------------------------------------------------------------------------

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[No: Does not follow] the guidelines for project length and complexity.

The module only checks for requirements for a webservices and introduces database tables for it. Since the database tables are there, the logic can be also keps on the local site. Also this seems to be quite risky for security, since the website maintainer knows so less about the functionality behind the database table logic.

Secure code

[No: List of security issues identified (see above and in the coding style and Drupal API usage hint).]

Coding style & Drupal API usage

https://www.drupal.org/node/101496 *

1. (*) Major finding, needs work
2. (+) Release blocker
3. Just a recommendation
4. ...]

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.

* status update *

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxchandanchaudhary23993...

I'm a robot and this is an automated message from Project Applications Scraper.

fixing tag. Please add your reviews to the issue summary.

Does not look like an actual security issue was identified here?

Automated Review

Review of the 7.x-1.x branch (commit 2e70428):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) I noticed that ubercart contributed modules starts with uc instead of ubercart.
2. (*) You might consider putting the other code in pages.inc instead of .module files except the required hooks.
3. (*) On line no 48, 'access arguments' => array('access content') seems irrelevant, Ideally you can set access callback to be TRUE
4. (*) On line no 116-118, Your URLs should be absolute as Veritans would not understand it.
5. (*) On line no 168, there is spelling mistake for "Response" word, also use placeholder for print_r($notification)
6. (*) On line no 316, Default value is incorrect it will use the finish page url, Also consider adding validation for internal paths.
7. (*) On line no 250, incorrect code comment, it should be Implements hook_uc_payment_method().
8. Just a recommendation, You might use Countries module, instead of defining your own in data.inc file

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.

None of these issues should block approval:

Instead of checking the PHP version in your hook_requirements(), you should do it in your .info file. See
https://www.drupal.org/node/542202#php

I don't think you need to check for the json_decode() in your hook_requirements, because json_decode() is always present if PHP version is > 5.2.0, so the check on the PHP version is sufficient. Regardless, consider using drupal_json_decode() instead of the PHP function.

In your hook_uninstall(), it is much better to use variable_del() rather than db_delete(). If you insist on using db_delete(), then you must invoke cache_clear_all('variables', 'cache_bootstrap'); after the db_delete().

I suggest your debug variable should default to FALSE - right now you are turning on debugging by default when the module is installed. I think that is bad practice, because the admin might not notice that debugging is on and might be saving/displaying debug information unintentionally by default.

You still have several spelling mistakes - you should run a spell checker on your code. These are a few I found:
'Unfinsih Url'
'Finsih Url'
'Debugg'

Ubercart already provides a mapping between the 2-character and 3-character ISO 3166 country codes using the Ubercart API function uc_get_country_data(). You do not need to have your own .data.inc file for this data. Using the Countries module, as a previous reviewer suggested, is overkill. (The Countries module is not an Ubercart dependency because the Countries module doesn't supply all the country data needed by Ubercart, and didn't even provide the minimal necessary data at the time Ubercart D7 was released. It has improved dramatically in the past 4 years, but it still lacks some necessary information.)

To address the previous reviewer's suggestion:

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Note that writing tests for web services like payment methods is extremely problematic because any reasonable tests would need to make live API calls to the web service, which requires API credentials built into the test functions and to be distributed as part of the module. Obviously that is only possible if there are generic 'test' credentials that can be made publicly visible and distributed with the module. In my experience, payment providers almost NEVER have public test credentials - all credentials, even test ones, are private and owned by each individual user. Because of this, the Veritrans API itself can't be tested with Simpletest - the only thing that can be tested is the payment process, and that is tested in core Ubercart.

manual review:

1. project page is too short, see https://www.drupal.org/node/997024
2. uc_veritrans_requirements(): mentions "The testing framework could not be installed because ..." copy & paste error? ;-)
3. "watchdog('vt_web', 'Something went wrong, please contact site administrator.');": this is not a useful log message. This will not be displayed to end users, but to admins, so you should include useful information such as IPN or transaction details.
4. uc_veritrans_vt_web_payment_notification(): this is vulnerable to XSS exploits. The notification is logged unsanitzed with the "!" placeholder to watchdog(). Since that can be arbitrary user provided input you need to sanitize it with the appropriate "@" or "%" placeholder. Make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. Fatal error: Class 'Veritrans_Notification' not found in uc_veritrans.inc on line 86. Looks like the library inclusion does not work?
6. Fatal error: Call to undefined function uc_cart_complete_sale() in uc_veritrans.inc on line 131. Looks like a missing dependency to uc_cart?
7. Fatal error: Call to undefined function uc_price() in uc_veritrans.inc on line 134 . Looks like that function was removed in Ubercart?
8. uc_veritrans_vt_web_payment_notification() has no access protection at all? Anyone can send a faked payment JSON request there and get their order paid? I exploited this with the following script, which will get my order number 2 paid:

<?php

$data = array(
'order_id' => '2',
'transaction_status' => 'capture',
'fraud_status' => 'accept',
);

$data_string = json_encode($data); 
 
$ch = curl_init('http://drupal-7.localhost/vt_web/notify');
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "POST"); 
curl_setopt($ch, CURLOPT_POSTFIELDS, $data_string);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_HTTPHEADER, array(
  'Content-Type: application/json',
  'Content-Length: ' . strlen($data_string)) 
); 
 
$result = curl_exec($ch);

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

project page is till too short, please update https://www.drupal.org/sandbox/chandan.chaudhary/2399385

Hi Kalusi,
Thanks for your valueble comment.
I have updated project page.

fixing tags.

Thank you for your reviews. Please add all of them to the issue summary so that we can better track them.

Review of the 7.x-1.x branch (commit a3f69bc):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/uc_veritrans.inc
  ---------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   151 | ERROR | [x] Spaces must be used to indent lines; tabs are not
       |       |     allowed
   151 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 3
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "drupal_set_message($e->getMessage(), 'error');": are you sure that the exception message is already sanitized? I would use check_plain() here if unsure.
2. Why are the Success/Error callback functions all empty except for a confusing message? Please add comments.

But otherwise looks good to me now. Assigning to patrickd as he might have time to take a final look at this.

no objections for a week, so ...

Thanks for your contribution, chandan.chaudhary!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

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 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.