HappyCaptcha

Hi

Your code is in the master branch. The master branch should be clean http://drupal.org/node/1127732

Regards,
jack_tux

As pointed out above, you should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.
Remove the license file as well as the timestamp, version and project keys from your info, they will be added automatically.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxalexyang1522380git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Looks good, the only thing left for now is the project page, please give some more information on it (installation instruct, usage, features..)

i have install this module with Captcha module. This module is working perfectly.
It would be nice if you could add a setting for hint image where we have an option to select the display image style. Now the images is showing in fixed size of 350x200.

Hi Alex,

I have just finished reviewing your module and here are my findings:

There are no license issues with this module as there are no third party files included.

Coding standards are generally good and an automated review did not find any issues.

You appear to have an empty function in happycaptcha.admin.inc (at the bottom of the file there is a validation function with no code in it).

As you have a configuration page, have you considered adding:

configure = admin/config/people/captcha/happycaptcha

to your .info file? This would make the config page link available under the admin/modules/list page.

Other than updating your project page with more info, I really don't see any other problems with this module.

It's RTBC for me.

1. You may use some HTML tags for your project page

2. $form['happycaptcha_challenge_fg'] = array(
       '#type' => 'textfield',
       '#default_value' => variable_get('happycaptcha_challenge_fg', 'FEFEFE'),
       '#title' => t('Challenge text color #'),
       '#required' => TRUE,
     );

   You can use #prefix for prefixing the field with '#' (maybe also #element_validate for validating whether it's an 6/3 char long string with letters from A-F)

3. You're only using variables with static names, so please consider deleting them with variable_del() instead.
4. fsockopen("happycaptcha.com", 80); You should check whether it makes sense to put a custom timeout parameter here, otherwise the config timeout will be chosen ($timeout = ini_get("default_socket_timeout");) so it's blocking the site until the timeout is reached when happycaptcha is not reachable.
5. happycaptcha_captcha_validation(): the indentation of this function is 8 spaces instead of 2.
6. happycaptcha_captcha_validation(): I think it's much easier (and best practice) to choose drupal_http_request() for doing this request.
7. $html = "<script>document.write('<script type=\"text/javascript\" src=\"http://happycaptcha.com/happycaptcha.js.php?fg=$happycaptcha_challenge_fg&bg=$happycaptcha_challenge_bg&r=' + Math.random() + '\"></sc' + 'ript>');</script>";
   $happycaptcha_challenge_fg, $happycaptcha_challenge_bg: These variables are not filtered before they're printed out and therefore this is an XSS security issue. please add check_plain() here.

added project page

great, don't forget to switch back to needs review

This is outside the scope of review for security, etc, but seems worth noting. Where you are doing this:

        // Check if HappyCaptcha is available and show Math if not.
        $connect = @fsockopen("happycaptcha.com", 80, $errno, $errstr, variable_get('happycaptcha_timeout', 5));
        if (!$connect) {
          return captcha_captcha('generate', 'Math', $args);
        }
        fclose($connect);

This only tests whether the webserver is listening on port 80 - it says nothing about whether the service is functioning. Wouldn't it be better to have a publicly available alive service that could be called through a higher level function? e.g.:

$response = drupal_http_request("http://www.happycaptcha.com/alive", array('timeout', 5));
if ($response->code != 200 || $response->data != 'some short string indicating that the service is alive') {
  return captcha_captcha('generate', 'Math', $args);
}

You could then provide a response that more accurately describes whether or not the happy captch service is available.

---

It would also be good to allow the user to set the timeout on the above check as a variable in the admin settings form - 5 seconds is a long time to wait on a page component to decide not to load if its service isn't running.

---

Security: this composite URL in the Javascript should be run through check_url(), since it's assembled from parts:

        $challenge_fg = check_plain($happycaptcha_challenge_fg);
        $challenge_bg = check_plain($happycaptcha_challenge_bg);
        $html = "<script>document.write('<script type=\"text/javascript\" src=\"http://happycaptcha.com/happycaptcha.js.php?fg=$challenge_fg&bg=$challenge_bg&r=' + Math.random() + '\"></sc' + 'ript>');</script>";
        drupal_add_css($happycaptcha_css_location, array('type' => 'external'));
        

You do run a regex check on these included values, but you should still use check_url() on the whole thing as well as check_plain() on the included parts.

---

Which leads to the regex check on the color, that actually lets you enter anything provided it's postfixed with a color code.

  if (!preg_match('/[a-f0-9]{6}$/i', $element['#value'])) {
    form_error($element, t('Bad color value'));
  }

So if you want to force six character color codes absolutely, that should be:

  if (!preg_match('/^[a-f0-9]{6}$/i', $element['#value'])) {
    form_error($element, t('Bad color value'));
  }

Does it have to be 6 character hex, or could you let users enter 3 character codes and color codes also?

---

You might look into the jquery_colorpicker module, which would be a nice and easy addition to the admin UI here - have the user use a UI to pick the color and then not have to worry about making them think about codes. It's pretty slick.

---

You might also look at Cache External Files with regard to using the external CSS file for the captcha - that may help with some of the slowness that tends to come with adding third party items to page.

---

A trivial note - changes nothing in your implementation. 'arguments' changed to 'variables' in D7:

/**
 * Implements hook_theme().
 */
function happycaptcha_theme() {
  return array(
    'happycaptcha_custom_widget' => array(
      'arguments' => array(),
    ),
  );
}

Should be:

/**
 * Implements hook_theme().
 */
function happycaptcha_theme() {
  return array(
    'happycaptcha_custom_widget' => array(
      'variables' => array(),
    ),
  );
}

---

Aside from that, it looks good.

You'll need to push whatever changes you made to the drupal.org repository - I'm not seeing them.

Manual review:
You will neeed: refer http://drupal.org/node/1011698

1. A link to your project page.
2. A direct link to your git repository (git clone ...).
3. Please specify if it's for Drupal 6 and/or 7.
4. always change status to 'needs review' after updating

update

added sanbox link

Assuming rogical is talking about the post at the head of this thread, you might look at my current project submission as an example of the form:

https://drupal.org/node/1488292

update

Manual review - issues in happycaptcha.module file:
1. You don't have description for the hooks you created, follow the link below and add description.
http://drupal.org/node/1354#hookimpl
2. Why are you using URL encode to construct URL - Line 119?

Tested it and it was working well. Tested the custom CSS file, and that worked.

Found just a couple of minor things.

In happycaptcha.module your hook_help, you have a couple incorrect links. admin/user/captcha/happycaptcha should be admin/config/people/captcha/happycaptcha. admin/user/captcha should be admin/config/people

line 54 in happycaptcha.module, the comment should say hook_permission() rather than hook_perm().

there is still contents in master branch as per http://ventral.org/pareview/httpgitdrupalorgsandboxalexyang1522380git ..try delete master branch , here is the instruction http://drupal.org/node/1127732

manual review
installed the module and tested working in registration form .. working fine :)

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 .
Delete master branch:

git branch -D master
git push origin :master

I'd like to have a screenshot on the project page.

return captcha_captcha('generate', 'Math', $args);
it's probably not the best solution to always fall back to the math captcha,
maybe rather add a new option as select box and let the user choose which captcha to fall back to.

        $challenge_fg = check_plain($happycaptcha_challenge_fg);
        $challenge_bg = check_plain($happycaptcha_challenge_bg);
        $random_num = rand();
        $happycaptcha_url = "http://happycaptcha.com/happycaptcha.js.php?fg=$challenge_fg&bg=$challenge_bg&r=$random_num";
        $happycaptcha_url = check_url($happycaptcha_url);

I think better here would be:

 $random_num = rand();
 url("http://happycaptcha.com/happycaptcha.js.php",
  array(
    'external' => TRUE,
    'query' => array(
      'fg' => $happycaptcha_challenge_fg,
      'bg' => $happycaptcha_challenge_bg,
      'r' => '$random_num',
    )
  )
);

same in your happycaptcha_captcha_validation(), also there you can use url()'s query feature

before you try to get the ->data of the resonse object you should make sure that ->error does not exist.

But nothing serious, just suggestions, therefore

Thanks for your contribution!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.