The project provides an integration between Drupal Commerce and the GoCardless.com payment service. More details about what it does are provided on the project page at https://www.drupal.org/project/commerce_gc_client. The module is called 'GoCardless Client', because sites that use it are 'clients' of Seamless-CMS.co.uk, which is a partner of GoCardless.com. As a partner Seamless-CMS receives a percentage of all revenue collected by GoCardless from payments created. It also means that any API calls created by client sites need to be channelled through Seamless-CMS rather than being made directly to GoCardless. One advantage of this to the client site is that no 3rd party libraries are required, and installation is simple. The partnership arrangement does not cost client sites or end customers any extra, and it is intended that the income generated from this will make the development and maintenance of this project sustainable, so security issues, bugs and feature requests can be responded to efficiently.
The project has evolved from an initial GoCardless integration written for Ubercart in Drupal 6. The initial module connected directly with GoCardless, rather than via Seamless-CMS. This was followed by a Drupal 7 version, which operated in the same way. After obtaining partnership status with GoCardless the module was once-again re-written to work as a client of Seamless-CMS. It was then ported in Drupal 8.
D7 Commerce GoCardless Client is therefore the 5th evolution of the project, and a D8 version is planned for the near future. With each rendition, the functionality has expanded, and the code has improved. So although the Commerce version of the module is very new, much of the coding has been around for a while and has been tried and tested on a range of live sites.
There is an existing Commerce GoCardless module available for D7 and D8 at https://www.drupal.org/project/commerce_gocardless. The other module hasn't received very much development since it was released five years ago, and has limited functionality compared with this one. Also GoCardless has moved on considerably since the other module, which uses the GoCardless API v.2, whilst Seamless-CMS client sites integrate with GoCardless via API v.3.
Within GoCardless API v.3, there are two kinds of payment service available: Subscriptions and One-off Payments. Subscriptions are automatic recurring payments, and work well for users that want to take the same payment on a regular basis (for instance £5 per week, or €20 on the first of each month). One-off Payments enable the client site to trigger a payment against a direct debit mandate at any time, so you can charge your end customers ad-hoc amounts.
GoCardless is expanding and the module is capable of working with the 7 currencies and 26+ countries currently available through GoCardless. Currencies supported are GBP, EUR, SEK, AUD, NZD, DKK and CAD. Also available in the near future: Brazil and United States.
Features of the module include:
- Subscriptions for automatic recurring payments
- One-off payments for client-side ad-hoc payment creation
- Fully configurable pre-defined recurrence rules on a per product basis
- A configurable field to allow customers to choose their own recurrence preferences
- Client-side recurrence triggering for One-off payments
- No libraries - easy installation
- Available in an expanding range of currencies and countries - even if your store only uses one currency
- Tools for managing subscriptions and one-off payments within existing orders / mandates
- A range of hooks to allow other modules to alter or react to events, such as mandate or payment creation
- GoCardless webhooks providing complete integration with site and customer mandates
- A pre-defined price field and rules for out of the box Flat Rate shipping solution
Project link
https://www.drupal.org/project/commerce_gc_client
Git instructions
git clone --branch 7.x-1.x https://git.drupal.org/project/commerce_gc_client.git
PAReview checklist
https://pareview.sh/pareview/https-git.drupal.org-project-commerce_gc_cl...
Comments
Comment #2
apadernoThank 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.
Comment #3
apadernoComment #5
roblogHi. Please can we re-open this issue. I would have responded to it months ago, but for some reason I did not get emailed any notification that you had responded to it, and did not realise until yesterday. I've worked through all the issues in the PAReview, and there are just a few left which I believe are false positives and / or problems with Coder.
Comment #6
roblogI see why I didn't get any email notifications now - I needed to click the 'Email notifications' link at the top of https://www.drupal.org/project/issues/projectapplications and enable notifications.
Comment #7
apadernoComment #8
klausiThanks 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.
Comment #9
roblogHi. Thanks for the review, I'll get on with the stuff that you have listed.
Comment #10
roblogHi Klausi, I see that hash_equals() is only compatible with PHP >= 5.6.0. There are some suggestions in the comments here about how to approach this for older servers: https://www.php.net/manual/en/function.hash-equals.php
Have you any preferences about this?
Comment #11
roblogHI. The module has been given a thorough review and I have addressed all the points listed by @klausi:
I really appreciate the support that you are supplying here! Please let me know what else is needed.
Comment #12
apadernoComment #13
roblogComment #14
klausiReview of the 7.x branch:
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!
Comment #15
roblogThanks for your additional review Klausi. I'll get straight on to the points that you have raised.
Comment #16
roblogComment #17
roblogHi again Klausi. I've addressed the three issues that you highlighted:
1. I've added the menu access callback function commerce_gc_client_mandate_cancel_access() to ensure that the mandate cancellation form is properly protected.
2. I've ameded commerce_gc_client_menu() to mark the paths with additional parameters.
3. I've checked and amended several docblocks with incorrect references to hook_form_validate() and hook_form_submit().
Thanks again for your most helpful comments.
Comment #18
er.garg.karanReview for the release 7.x-1.0
Comment #19
apadernoComment #20
roblogHi Karan, thanks for the input, I'll make the changes that you have highlighted.
Comment #21
roblogHi Karan, I've addressed the following points from your review:
1. Remove unnecessary empty lines in the start every function.
- Done.
2. Avoid using names like '$calcs_arr', '$calcs', '$calc_data', 'sched_data', Try using meaninful names for variables
- Gone through all the code and ensured variables have meaningful names.
3. Requires proper function level commenting which includes proper information about function params, exceptions and return
- Gone through all the code and improved the standard of the API documentation and commenting.
4. 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
- Have fixed this.
I think that you must have been reviewing an older revision of the code, as the following points that you addressed had already been fixed:
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
Remove commented code on line 98:10, 834:10 in file commerce_gc_client.mandates_form.inc
Remove empty line 145, 206, 343, 393, 437, 483, 551, 745, 792, 805 in file commerce_gc_client.mandates_form.inc
Unused variable $update on line 343:7, $update on 349:6 in file commerce_gc_client.module
The most recent PAReview is at https://pareview.sh/pareview/https-git.drupal.org-project-commerce_gc_cl...
Thanks again for your help.
Comment #22
roblogComment #23
klausicommerce_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.
Comment #24
roblogHi Klausi.
> I think you need to check if the order owner equals the current user, right?
Unless I am missing something, this is what the code does.
arg(2) is the order's owner, and $user->uid is the current user.
Comment #25
roblogAhh yes sorry I see what you mean now. Thanks for pointing this out, I'll fix it.
Comment #26
roblogHi again Klausi, this is fixed now. See https://git.drupalcode.org/project/commerce_gc_client/blob/7.x-1.x/comme...
Comment #27
apadernoRemember to change status, when the reported issues have been fixed.
Comment #28
roblogComment #29
klausiThanks for the fixes!
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.
Comment #30
roblogHi Klausi, thanks for bumping this back again with your improvements. I've re-read the docs on handling text securely, and gone through the code and checked that it is only outputted data that has check_plain() applied.
commerce_gc_client_menu(): there are three menu items that refer to commerce_gc_clients.mandates_form.inc. Two of them were correct, having been updated after your initial review, but I had overlooked the third menu item which was still pointing to commerce_gc_clients.mandates.inc. Thanks for pointing this out!
variable_set(): This was some legacy code which I had failed to remove. I was using hook_date_formats() to define a 'gocardless' date format, and since in D7 these formats don't seem to be lockable, the variable_set() function was there to ensure that if anyone had changed this date format manually, it would be changed back, to ensure there were no errors with the GoCardless API. I checked the code, and the 'gocardless' date format was only being used twice, so I have used the date() function in these two places instead, and have removed the 'gocardless' format from commerce_gc_client_date_formats(). Also I've removed the redundant variable_set() function as you suggested.
Comment #31
roblogComment #32
apadernoThe few implementations I found of
hash_equals()
for PHP versions that don't provide that function simply returnFALSE
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()
?It seems a rather convolute way to avoid a set of
if()
statements that just makes the code harder to read.It is necessary just a single line, instead of those.
Comment #33
roblogHi kiamlaluno, thanks for the review.
* I've moved the two functions out of commerce_gc_client_webhook() as you suggested, and made them into functions of their own. I've left the new functions in the commerce_gc_client.webhook.inc file though since this is the only place that calls them, and they are not hooks.
* I've replaced all three instances of the header() function with drupal_add_http_header() as you suggested.
* I've replaced the slightly convoluted switch statement with a series of if statements, as you suggested, to make the code easier to read.
* I'm not sure how to go about your last suggestion to put the if statement on one line. I tried the following code and I get a PHP syntax error: "ParseError: syntax error, unexpected 'return' (T_RETURN) in drupal_load()"
$args[2] == $user->uid && $args[2] == $order->uid ? return TRUE : return FALSE;
How would you do it please?
Comment #34
roblogComment #35
apadernoIt's enough a line like the following.
That is equivalent of the code you used.
Comment #36
roblogYes that is much more compact, thanks. I've pushed the latest, and am ready for review again.
Comment #37
apadernoThank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
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.
Comment #38
roblogHi kiamlaluno, that's great! Many thanks to you and the other reviewers. I'll read the links you have provided, and hopefully make some time in the future to help out with some reviewing myself!