Commerce Paybox

Hi,

brief manual review:

• Take some time to read into the commenting standards for your non-hook functions.
• Hint: In your hook_install implementation you could also pass the bare array without the manual imploding (serialization is handled internally by the Drupal variable system). Just to let you know.

Hi,

ok, just take the hint on serialization as a general one. Of course you may store into the variables whatever you suppose is best ;)

Let's see what the old stagers will still find ;)

Good luck!

Oh, wait... you are still on the master branch, but you should move to a major release branch; see http://drupal.org/node/1127732 for details. Sorry I missed that in the first round.

Hi,

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./commerce_paybox.module:176:    'PBX_MODE'        => 'DO_NOT_ALTER_THAT', // will be set to 4 after params_alter (command line mode)
  

• ./commerce_paybox.api.php: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function hook_commerce_paybox_params_alter(&$params, $order, $settings) {
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./commerce_paybox.module:335:  $pos = strpos($q, '=', $pos) + 1;
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./commerce_paybox.install ./README.txt ./includes/commerce_paybox.admin.inc ./commerce_paybox.api.php ./commerce_paybox.module ./commerce_paybox.info ./tests/commerce_paybox.test
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Remark: The "operator" issue in 335 is a false positive.

HTH, dave

Hi.

as I am obviously being adressed with your recent comment, you may probably want to consider a few things.

• I am trying to help out here as best as I can and I try to stick to the official processes.
• Wouldn't you agree that it saves time for all contributors if we first fix the obvious issues that come out with automated reviews before going in depth?
• Feel free to join the reviewers as well. Everyone who has an understanding of the process is not only invited to do so but also badly needed here. You will, btw, see that when you check with the issue queue. You may also become aware that there are projects waiting not just 5 days - as yours does - but some still need review after 5 *months*. Wouldn't you also agree that, just for the sake of chronology, they deserve an in-depth review at first?

I can understand your impatience. Honestly. And I would be as happy as many others if there were dozens of people here making this approval a question of a few hours. (As I am, besides, still waiting myself.)

Of course this decision is up to anyone himself, but *I* decided to help myself speeding up this thin instead of complaining. Sorry, but I felt that had to be said. Just think about it.

Thank you!

Following and fixing up any Coding Standards issues means that manual review is made much easier and we don't get stuck on them.

master branch

If you're no longer using the master branch, please remove all files from it and commit only a README file telling people how to change to one of the available branches. See Moving from a master branch to a version branch.

chicken and egg - hook_requirements

The module requires its variables to be configured before it's even installed. In this particular case, you should probably not block install but only make the module non-functional until the settings have been set. Keep the hook_requirements runtime limits so that admin are warned about the fact that they need to configure the module before being able to use it.

requirement

hook_requirements should also check for openssl. All of these requirements should be in their own sub-heading on the project page. Entity API is also a required module which should be mentioned? The .info file dependecies[] means that it doesn't need to go into hook_requirements obviously, but it should be on the project page, unless it's a requirement of commerce. In which case it probably doesn't even need to go in the info. Also need to mention that Safe Mode can't be enabled.

commerce_paybox_paybox_servers

do not implode/explode in set/get. Just store the array. variable_set/variable_get will serialize the array automagically.

commerce_paybox_auto

Can the arguments to this not be passed via arguments? Use arg()? The code in commerce_paybox_check_sign in insane considering url opts/args are handled by drupal automatically. What is the format of the URL that it requires such unique and special treatment?

commerce_paybox_get_paybox_error_msg

Are these error codes really strings prefixed by 0's? Surely numerical comparison would work better.

form validation

commerce_paybox_settings_form needs validation. Anything can be entered into the text fields but I assume that there is some kind of required format that can be checked against.

construct command line

use escapeshellarg to escape args.

cgi app

Is there a developer API available that would mean you could get rid of the cgi app and make it drupal native? I don't know how many locations are limited by Safe Mode, but it wont be zero. Since this is a cgi app, if should possibly also be run by calling drupal_http_request instead of direct execution.

// Something goes wrong with GCI execution...

watchdog to inform admin?

commerce_paybox_redirect_form_validate

Why does this return FALSE on error instead of form_set_error? Returning something from a validation function doesn't do anything, it will simply continue onto the submit function.

commerce_paybox_params_alter

This hook does not seem to be implemented anywhere, and the api.php documentation for it is ... sparse.

The raw usage of $_GET is all fine.

Hi,

How can I download this module ? I think the link is broken...

Thanks

As this is still a sandbox project, it has no downloadable releases yet.

You have to clone it with git or browse the repository and download the current snapshot.

Generally I would not suggest you to use a sandbox module if you don't know what your doing ;)

I mostly agree with #10, i'll add few points:

* in commerce_paybox_auto(), when you check the return value of commerce_paybox_check_sign() you should insert an entry in the watchdog if the return code is a failure.
You should throw an exception instead of echoing an empty string to stop the execution of the page if nothing happened.

* in commerce_paybox_ppps_submit_form() why do you fetch the default settings and don't do anything with it?

This module looks good ! I hope it will be stable enough to test it on a real project in a few weeks.
Thank you for your work.

No need for a demo site there, that would imply having a Paybox demo account and it usually don't happen.

Hi
What's new with your module?
Where can I download the current version.
best regards

Looks like you've got a few cases where the strict coding standards will trigger; mostly using spaces to make things look pretty. Run the module through the coder module on strict to deal with any of those. I believe there's a website which will do this for you if you ask it nicely.

commerce_paybox_auto

printing nothing and then calling exit doesn't allow for drupal to finish up properly. If you don't want the theme rendered around the outside of the content you've printed, return NULL from the function to tell the theme engine that you've already done all of the theming yourself already.

commerce_paybox_redirect_form_validate

If this is actually hook_redirect_form_validate, then it should be commented appropriately "Implements CALLBACK_commerce_payment_method_redirect_form_validate()." so that the parser knows it's API hook/callback and so does everyone reading it. Ditto for all the other callbacks.

commerce_paybox_settings_form

The inputs on this look like they should be validated prior to being accepted as these values do have requirements on them; pbx_site looks like it needs to be an integer. The documentation doesn't seem to say if CALLBACK_commerce_payment_method_settings_form has an automatic _validate handler, or if you need to include #element_validate and provide a function to each of them. Either way, unless a form is full of radios/selects/checkboxes or some other input where the data is checked for you, validation should be a crucial first step of any form.

commerce_paybox_check_sign

do you have an example of the query string that gets sent to this function? the processing looks to be quite over the top for something that should already be included in $_GET. Passing a global variable to it. The function also doesn't seem to trap or deal with the error state that the key file is missing or undefined, but you are setting the default to the empty string instead of the included by default key file.

commit message

Please refer to Commit messages - providing history and credit about giving yourself some credit, and following the correct format for each commit message.

he rest of the changes you've made look really good.

non-blockers

commerce_paybox_check_sign

I'm afraid I don't have time to track down the developer docs for Paybox, I'm just looking at the code and thinking it's a strange way to handle the request URI if they were already automatically processed GET vars. If you could provide an example I could judge easily whether there might be a better way to deal with it. For now though, it can be as it looks secure.

Regarding the default key fallback, you can simplify that code by using variable_get('commerce_paybox_pubkey_path', drupal_get_path('module', 'commerce_paybox') . '/pubkey.pem'); initially, negating the need for a double wrapped version. variable will return the 2nd parameter, the default value, if the variable is missing. The check on the settings input form has ensured that the file in the variable was in existence when it was set.

You also seem to call file_get_contents($key_file) a lot in that function. Once should be enough.

Would something like this work?

  $key_file = variable_get('commerce_paybox_pubkey_path', drupal_get_path('module', 'commerce_paybox') . '/pubkey.pem');
  if ($key_file_content = file_get_contents($key_file)) {
    if ($key = openssl_pkey_get_public($key_file_content)) {
      return openssl_verify($data, $sig, $key);
    }
  }

  $msg = 'Cannot read Paybox System public key file (@file).';
  $msg_vars = array('@file' => $key_file);
  watchdog('commerce_paybox', $msg, $msg_vars, WATCHDOG_ERROR);
  return FALSE;

Anyway, the rest of this module looks like it's all well and good.

/me puts on his suggestion hat.

There's actually a PHP function that will do all that parsing for you which will handle any strange things like parameters moving around or anything else that might upset the positional parsing.

http://www.php.net/manual/en/function.parse-str.php

parse_str($query_string, $args);
print_r($args);

gives the following from what you have posted

Array
(
    [q] => commerce_paybox/auto
    [txnid] => 61
    [error] => 00004
    [sig] => bfq0uj3553qkYln/6p/NShOpE9LGmrmPQWlWKziDR3k/aULYviqiF6Ez/FugTuH2E1TYpiuVFtf455GlRWIhyaPC6XdGc5KAtBzC8SehWOiZ3Cwf6PPEIhDsDsWvRVajjsx9wTp5XL2C7+LoKtftHUikOZic6jr62cnr1wAieOk=
)

You can then use $arg['sig'], but it looks like the data "txnid=61&error=00004" needs to be reconstituted and http_build_query is really good at that.

My testing code ended up being the following. If someone is sending your validation function strange things, what happens when the positional stuff doesn't match up to what is passed? ie, they're screwing with it. The function is getting passed a global variable anyway, it could source it itself.

function commerce_paybox_check_sign($query_string) {
  // Split the supplied string just like PHP does automatically
  parse_str($query_string, $args);
  print_r($args);

  // These args are required to continue
  if (!(isset($args['txnid']) && isset($args['error']) && isset($args['sig']))) {
    return FALSE;
  }

  // base64_decode can fail and return FALSE.
  // parse_str has already run urldecode (undocumented)
  $sig = @base64_decode($args['sig']);
  if (!$sig) {
    return FALSE;
  }

  // rebuild data for signature
  $data = array(
    'txnid' => $args['txnid'],
    'error' => $args['error'],
  );
  $data = http_build_query($data);

  print_r(array($args, $sig, $data));
}  

Anyway, that's just a suggestion. This module is still RTBC and we should probably create an issue in the module issue queue for something like this.

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• hook_commerce_paybox_params_alter(): no example is bad, you should provide a simple code snippet.
• Use the function ip_address() instead of $_SERVER['REMOTE_ADDR'] (added a sniff to drupalcs that detects that, too).

But that are just minor issues, so ...

Thanks for your contribution, IcanDivideBy0! 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 mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

Ok, that's a not a very good interface from Paybox then. If the parameters can be anything, it should include a parameter on the end specifying which parameters it signed, and in what order. That method is fairly common - eg DKIM.

In the mean time, this would work.

  // rebuild data for signature
  unset($args['q']);
  unset($args['sig']);
  $data = http_build_query($args);

If they really are going to simply throw a signature on the end of whatever they call back to your site with, then a preg_match would also work.

function commerce_paybox_check_sign($query_string) {
  if (preg_match('/q=.*?&(.*)&sig=(.*)$/', $query_string, $matches)) {
    $data = $matches[1];
    $sig = urldecode($matches[2]);

    print_r(array($data, $sig));
  }
}

Anyway, you're on your way.

Looks like your project is stuck without a release tag.

Types of releases for full projects

Release naming conventions

Creating a project release

Creating a branch or tag in Git