[D7] Commerce GoCardless Client

Thank you for your contribution!
I am adding the PAReview checklist link. If you haven't done it yet, please check the reported issues, and fix the code as indicated. Don't pay attention to the false positives the checklist could contain.

Next, the reviewers will check the project code, and report here what needs to be changed.

Thanks for your contribution!

Review of the 7.x branch:
* Can you remove all the *.orig files from git? They are confusing and just blow up the number of files to audit.
* $_SESSION['line_item_id_': all variables you use in the sessio0n should be prefixed with your module name to avoid name clashes with others.
* $text_top = t('<h3><b>Administrate <a href = "@url">@title</a></b></h3>': markup tags should be outside of t() where possible.
* commerce_gc_client_mandates_form(): this function is almost 700 lines long and very hard to review. Can you split it up into more meaningful parts?
* "'#value' => 'Cancel subscription at GoCardless',": all user facing text must run through t() for translation. Please check all your strings.
* "'#title' => check_plain(t('Amount') . ' ' . commerce_currency_get_symbol($commerce_currency_code)),": do not concatenate variables to translatable strings, use placeholders with t() instead. Please check all your t() calls.
* commerce_gc_client_mandates_form() $payment_rows: They are built from a payment response received from an API, right? So we have to consider them untrusted user provided input, for example $payment->description. You then put these values into a table, but I cannot see where you sanitize the values against XSS. Maybe I'm missing something?
* commerce_gc_client_webhook(): You should not compare crypto signatures with "==", use hash_equals() instead to protect against timing attacks.
* commerce_gc_client.mandates.inc: this file does not exist but is referenced in hook_menu()?

So although I did not try to directly exploit those things and they don't seem super dangerous security issues I would suggest that you rework the module a bit and fix them. Please also do a thorough internal review of the module at your company so that we have it easier the next time we look at this.

Review of the 7.x branch:

1. commerce_gc_client_mandate_cancel_form(): The path gc_client/mandate_cancel is missing access control. An attacker can specify arbitrary user IDs and order IDs here that should be cancelled. You need to authenticate those requests to ensure only admins or the order owner can do that, I think? Not sure how this should work. Anyway, this is currently a security blocker. Please check all your paths that they are properly authenticated and that attackers cannot just change arbitrary order data like that.
2. commerce_gc_client_menu(): if you pass on parameters in your menu paths then you should mark them with "*" like gc_client/mandate_cancel/*/*/*.
3. commerce_gc_client_mandate_cancel_form_submit(): do block is wrong, this is not a hook implementation. This is a submit callback, see https://www.drupal.org/docs/develop/standards/api-documentation-and-comm... . Please check all your docblocks.

Otherwise I think this is almost ready, so if you clear up this last security issue then I think we should be good to go!

Review for the release 7.x-1.0

1. Remove unnecessary empty lines in the start every function.
2. Unused variable $rows on line 38:3, $db_update on 936:4, $delete_sch on 1129:6 in file commerce_gc_client.mandates_form.inc
3. Variable $line_item_title on line 57:113 in file commerce_gc_client.mandates_form.inc will throw a variable not defined warning if the foreach loop on line 41:3 is not executed
4. Remove commented code on line 98:10, 834:10 in file commerce_gc_client.mandates_form.inc
5. Remove empty line 145, 206, 343, 393, 437, 483, 551, 745, 792, 805 in file commerce_gc_client.mandates_form.inc
6. Avoid using names like '$calcs_arr', '$calcs', '$calc_data', 'sched_data', Try using meaninful names for variables
7. Requires proper function level commenting which includes proper information about function params, exceptions and return
8. Unused variable $update on line 343:7, $update on 349:6 in file commerce_gc_client.module

commerce_gc_client_mandate_cancel_access(): it seems like this function is missing a check for the order. An attacker can specify an arbitrary order ID that should be canceled. I think you need to check if the order owner equals the current user, right? This looks like an access bypass vulnerability currently and an application blocker.

Remember to change status, when the reported issues have been fixed.

Thanks for the fixes!

1. CommerceGcClientPartner::post(): why do you have a variable_set() call here? Seems unrelated and should be removed?
2. commerce_gc_client_redirect_form_submit(): you are calling check_plain() here on the value of a db_insert() call. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it (including any dangerous content they may have included). This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/docs/7/security/writing-secure-code/handle-text-i... . Please check all your db_insert() and db_update() calls.
3. Same in commerce_gc_client_webhook_payments(): no need to sanitize for XSS when you save the transaction, that should be done on output and not here.
4. commerce_gc_client_redirect_form_submit(): "'@interval' => check_plain($subscription->interval),": the @ placeholder with t() already runs check_plain(), so you are doing double escaping here. That is bad because it can result in broken text display.
5. commerce_gc_client_menu(): the referenced file commerce_gc_client.mandates.inc does not exist. I already reported this in my previous review, looks like you did not fix that yet? Can you go through all my reviews one more time and make sure everything is fixed?

Looks good to me otherwise, sorry to bump this back one more time. We want to make sure that you understand where to use check_plain() and where not. When this is cleared up then I think this is ready.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

    function hash_equals($known_string, $user_string) {
      $ret = 0;
      if (strlen($known_string) !== strlen($user_string)) {
        $user_string = $known_string;
        $ret = 1;
      }
      $res = $known_string ^ $user_string;
      for ($i = strlen($res) - 1; $i >= 0; --$i) {
        $ret |= ord($res[$i]);
      }
      return !$ret;
    }

The few implementations I found of hash_equals() for PHP versions that don't provide that function simply return FALSE when the strings have different lengths. This includes the polyfill used from Symfony.

Instead of defining a function inside another function, I would rather define it outside, possibly in the module file.

Is there a reason not to use drupal_add_http_header()?

  switch (TRUE) {
    case $action == 'created':
      commerce_order_status_update($order, 'processing', FALSE, TRUE, $log);
      break;

    case in_array($action, array(
      'submitted',
      'pending_customer_approval',
      'pending_submission',
      'customer_approval_granted',
      'customer_aproval_skipped',
      'transferred',
      'resubmission_requested',
    )):
      $order_wrapper = entity_metadata_wrapper('commerce_order', $order);
      $order->revision = TRUE;
      $order->log = $log;
      $order_wrapper->save();
      break;

    case in_array($action, array(
      'active',
      'reinstated',
      'replaced',
    )):
      commerce_order_status_update($order, 'completed', FALSE, TRUE, $log);
      break;

    case in_array($action, array(
      'failed',
      'cancelled',
      'expired',
    )):
      commerce_order_status_update($order, 'canceled', FALSE, TRUE, $log);
      break;

    default:
      $order_wrapper = entity_metadata_wrapper('commerce_order', $order);
      $order->revision = TRUE;
      $order->log = $log;
      $order_wrapper->save();
      break;
  }

It seems a rather convolute way to avoid a set of if() statements that just makes the code harder to read.

    if ($args[2] == $user->uid && $args[2] == $order->uid) {
      return TRUE;
    }
    else {
      return FALSE;
    }

It is necessary just a single line, instead of those.

It's enough a line like the following.

return ($args[2] == $user->uid && $args[2] == $order->uid);

That is equivalent of the code you used.

if ($args[2] == $user->uid && $args[2] == $order->uid) {
  return TRUE;
}
else {
  return FALSE;
}

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.