[D7] Commerce BluePay Hosted Forms

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.

Hi tallosoft,
I have manually reviewed the code and looks fine, but not installed in drupal. There are couple of warnings from the pareview test which can be fixed.

In commerce_bluepay_hosted_forms.module file,
On line 183, there is a warning from pareview.sh,
183 | WARNING | Only string literals should be passed to t() where possible.
Try concatenating the variable in drupal_set_message() with the quotes which might fix this warning.

In README.txt file,
52 | ERROR | Files must end in a single new line character
Enter the new empty line at the end of the file.

Hi tallosoft,

I installed your module into a non-kickstart Commerce test site. Here's a couple of things I noticed, that you might want ot take a look at:

• The payment method is enabled by default, meaning that it's automatically available at the end of the checkout after installing the module. If adding this payment method to a live site, this doesn't give you anytime to configure the module before deploying it.
• Striaght after enabling I navigated to /admin -> Store -> Configuration -> Payment methods -> Bluepay Host
  ed Forms Integration -> Enable payment method: Bluepay Hosted Forms Integration and I received the following error:

  "Notice: Use of undefined constant BLUEPAY_MODE_TEST - assumed 'BLUEPAY_MODE_TEST' in commerce_bluepay_hosted_forms_settings_form() (line 66 of /home/alex/workspace/commerce/sites/all/modules/2144659/commerce_bluepay_hosted_forms.module)."

• I also ran the Coder over the module at it produced the following (one of which you're already aware of):

Line 176: Use an indent of 2 spaces, with no tabs [style_indent_spaces]

         $order, $payment_method) {

severity: minorreview: sniffer_semantics_functioncall_notliteralstringLine 183: Only string literals should be passed to t() where possible [sniffer_semantics_functioncall_notliteralstring]

    drupal_set_message(t($not_configured), 'error');

severity: criticalreview: security_3Line 183: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs) [security_3]

    drupal_set_message(t($not_configured), 'error');

severity: normalreview: style_comma_spacingLine 313: missing space after comma [style_comma_spacing]

    ' id="iframe_bluepay" src="' . url($server_url) . '" />',);

severity: minorreview: style_indent_spacesLine 382: Use an indent of 2 spaces, with no tabs [style_indent_spaces]

         ucfirst(strtolower($_REQUEST['MESSAGE'])) . '<br /><br />';

Looks good otherwise.

Just re-checked the code and all looks good, except Coder is still grumbling, with the following:

Line 183: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs) [security_3]

    drupal_set_message(t('!notconfigured', array(

Looking at it, you could just change the drupal_set_message() code to be:

drupal_set_message(t('Bluepay Hosted Forms Integration is not configured for use.'), 'error');

Seeing as $not_working isn't used elsewhere.

Hi

The module looks nice, the only thing I think you should change is the invocation of http_bulid_query in .module:304:

$query_string = http_build_query($params);

Changing that one for this one:

$query_string = http_build_query($params, '', '&');

When calling http_build_query(), make sure the third argument is "&". Otherwise, your system may default to "&" which means something completely different to the server processing the request (i.e. apple=red&banana=yellow&pear=green).

Hi

Yes that's exactly what happened, the sing for & amp ; was escaped :-D

Good job!!!!

Removing review bonus tag, you did not all manual reviews, you just repeated the output of an automated review tool. Make sure to read through the source code of the other projects.

I haven't installed your module, but I had a look trough the Git repository.

Take a look at https://api.drupal.org/api/drupal/includes!common.inc/function/l/6, there are a few manually created links.

function commerce_bluepay_hosted_forms_handle_callback($order) {
has a few text strings that are not passed trough t(), and the output here should be in a template (imho).

Review of the 7.x-1.x branch:

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

  
  FILE: /home/klausi/pareview_temp/commerce_bluepay_hosted_forms.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   381 | WARNING | Only string literals should be passed to t() where possible
  --------------------------------------------------------------------------------
  

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. as the automated review says: t() should be around string literals where possible. You are defining $untranslated just one line before, and that string should run through t().
2. commerce_bluepay_hosted_forms_handle_callback(): so this has no access protection and I cannot see where you verify the incoming request that it is actually genuine and coming from the payment provider? Looks like anyone that has the order ID can successfully complete the payment with a malicious request? Looks like a security issue to me, can you explain where you verify the request? And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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