UC WePay

Please remove license.txt file. It wiil be added automatically.

Tagging.

• git release branch missing, see http://drupal.org/node/1015226
• info file: remove "version", this is added by drupal.org packaging automatically
• @file doc blocks are missing, see http://drupal.org/node/1354#files
• "// (GOODS, SERVICE, DONATION, or PERSONAL) For now we support only goods and service" Comments should be on a separate line.
• "// process only if user selected WePay as payment method": comments should start capitalized and end with a "."
• "@param $op" if you document parameters please add a description, otherwise this is useless.
• uc_wepay_do(): Always use "{}" around if/else constructs, even if they are just one liners. Also elsewhere.
• Are you sure you need cURL? Does drupal_http_request() not suffice?
• "switch ($checkout->state)": don't use switch() if you only deal with one case. Use if{} instead.

• uc_wepay_ipn(): where does $result come from?
• uc_wepay_do(): "} else {": else needs to be on a new line. Also in uc_wepay_admin_new_account_submit()
• "@param type $command": what is the type of $command? array? Please fix that.
• uc_wepay_admin_new_account(): I think you should add a check_plain() around $result['error'] and $result['error_description'], just to be sure not to output any crappy response. Also in uc_wepay_admin_settings() and maybe elsewhere.

uc_wepay_admin_settings(): there is still a "drupal_set_message($accounts['error_description'], 'error');" which should also use check_plain(), no?

Otherwise RTBC for me. Now while we are waiting for the access grants to create full projects would you be so kind to do a review of the other applications pending? Just pick one from this list: http://drupal.org/project/issues/projectapplications?status=8

Please take a moment to make your project page follow tips for a great project page.

uc_wepay_admin_settings(): there is still a "drupal_set_message($accounts['error_description'], 'error');" which should also use check_plain(), no?

Yes, that needs to be done prior to this becoming a full project.

Having a closer look, I'm still not completely happy with the security text sanitization. From http://drupal.org/node/28984

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. This means that conversions are performed when content is output, not when saved to the database (be sure to read the db_query() documentation on how to use the database API securely).

We don't have user typed input here, but input from a third party web service. We don't save to the database here, but we pass along the data. So you should not filter on incoming data (to preserve its original state), but you should filter immediately before outputting that data to the browser. You do the output filtering correctly now, but you still have check_plain() calls in uc_wepay_order(). I thought maybe you do it because the return value of this hook is printed unfiltered to the user, but when looking into uc_order.module I found that the return value of that hook is ignored anyway. Then I saw that you use the operation "submit", which is not even invoked in that module (what a mess!). I grepped after "('submit'" and traced it down to uc_cart.pages.inc, which does the invocation and ... passes unfiltered results to drupal_set_message().

Wow, so you are actually doing it right and my comment is pointless. However, I post this anyway to give greggles an example of how I do security reviews and as a basis for discussion.

Therefore, RTBC for me.

@Branislav: Maybe some modules want to output HTML in this messages, so check_plain() might not be wanted. filter_xss() might be an option, you could open an issue about that in the Ubercart issue queue. If that changes then you could remove your check_plain().

About change notifications: module maintainers should announce such important changes in the release notes. But of course it is always a good idea to follow changes in components that you depend on (Drupal core, Ubercart).

Thanks for your contribution, Branislav Bujisic! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.