[D7] Facebook comment notification

Fixed the git clone URL in the issue summary for non-maintainer users.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Pragna,

Automated Review looks ok.

Manual Review

Basic Application Checks
[Yes] - Ensure that the application contains a repository and project page link.
[Yes] - Ensure it´s not a duplication.
[Yes] - Ensure the user does not have multiple applications.

Basic Repository Checks
[Yes] - Ensure the repository actually contains code.
[Yes] - Ensure the applicant is working in a version specific branch.
[Yes] - Ensure branches and tags are following naming conventions.

Licensing
[Yes] - Ensure the repository does not contain a ‘LICENSE.txt’ file.
[Yes] - Ensure the repository does not contain any 3rd party (non-GPL) code.

Documentation
[No] You have a detailed README.txt please make sure the project page contains at least the same information. - Ensure the project page contains detailed information.
[Yes] - Ensure the repository contains a detailed README.txt
[Yes] - Ensure the code contains a well-balanced amount of inline-comments

Coding Standards & Style
[Yes] - If not done yet run an automated review and ensure there are no major issues.

API Review & Security
[No] - Ensure the applicant is using Drupals API correctly.

/**
 * Implements hook_menu().
 */
function fb_comment_notification_menu() {
  $items = array();
  $items['admin/config/facebook-comments-notification'] = array(
    'title' => 'Facebook comments notification settings',
    'description' =>
    'Configure Facebook comments settings like the Facebook App ID.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('fb_comment_notification_admin'),
    'access arguments' => array('administer facebook comments'),
  );
  $items['send/fb-mail-notify'] = array(
    'page callback' => 'fb_comment_notification_sendmail',
    'access arguments' => array('access content'),
  );

  return $items;
}

One of your hook_menu items, send/fb-mail-notify, does not have a title. As this menu path is also used through an ajax post request bound to a facebook event in jQuery it should also be of type MENU_CALLBACK.

I am also concerned here that no access validation is performed on this path. Any site that has this module installed is vulnerable to fake post requests and DDOS attacks. Drupal has a token api to create tokens and verify tokens which you could check in a custom access callback with drupal_get_token() and drupal_valid_token(). The drupal token api however relies upon user sessions so any site behind a reverse proxy like varnish would fail to cache because of the sessions. But in theory you can take a look at drupal_get_token() and generate a token based on something other than the session_id() and use a custom validation function.

function drupal_get_token($value = '') {
  return drupal_hmac_base64($value, session_id() . drupal_get_private_key() . drupal_get_hash_salt());
}

I am also concerned about this function:

/**
 * Settings of mail.
 *
 * Implements fb_comment_notification_sendmail.
 */
function fb_comment_notification_sendmail() {
  if (!empty($_POST['urlofpage']) && !empty($_POST['comment_msg'])) {
    $module = 'fb_comment_notification';
    $key = 'fb_comment_notification';
    $language = language_default();
    $params = array(
      '@url' => $_POST['urlofpage'],
      '@comment' => $_POST['comment_msg'],
    );
    $mail = variable_get('fb_comment_notification_admin_email', '');
    $send = TRUE;
    $from = variable_get('site_mail', 'admin@example.com');
    $msg = drupal_mail($module, $key, $mail, $language, $params, $from, $send);
    if ($msg['result'] == TRUE) {
      drupal_set_message(t('Your message has been sent.'));
    }
    else {
      drupal_set_message(
        t('There was a problem sending your message and it was not sent.'),
          'error');
    }
  }
}

I recommend you should at least use filter_xss() on $_POST['urlofpage'] and $_POST['comment_msg']. You could also add url validation to $_POST['urlofpage']. Not sure why you use drupal_set_message in this callback if it is access through an ajax call.

Thanks for your work!

Best Regards,
Helgi.

Hi pragna,

Also please add links to your manual reviews before tagging your application with the PAReview: review bonus.

Best Regards,
Helgi.

yep, please add manual reviews that you did to the issue summary before adding the review bonus tag.

Hi pragna,

The token workflow was only a suggestion. I don't think it would be a blocking issue but I think you should at least create a custom permission by implementing hook_permission() to give the site administrator the option of allowing/restricting access.

Best Regards,
Helgi.

Hi Pragna,

Thanks for contributing this module.
Automated as well as manual review look fine to me.

Hi Pragna,

1. I reviewed your module, you need to add email validation for Facebook comments notification settings also attached screentshots.
http://cgit.drupalcode.org/sandbox-pragna-2580865/tree/fb_comment_notifi...

2. Also as an example,please upload the screenshot of your project page.
https://www.drupal.org/sandbox/pragna/2580865

Thanks
Shakti

Comment #14

That validation and the screenshot for the project page are surely not application blockers, anything else that you found or should this be RTBC instead?

Hi Pragna,

Please Move your admin setting form functionality in .admin.inc.

Thanks.

Using an admin.inc file is optional and surely not an application blocker. Anything else that you found or should this be RTBC instead?

There should be also uninstall hook in which you will delete all of the variables (variable_get()) by variable_del($name).

Thanks.

Hi,

Remove the variable fb_comment_notification_admin_email when your module has uninstall.

Refer:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

1 warning that affects 1 line of code i.e unused variable $msg.

Hi pragna,

I did a manual review.

Currently module is triggering emails when the users comments through Facebook comments block. It is because you are adding/appending your js to the pages through Facebook comments block.

/**
 * Add fb.init to send mail using ajax.
 *
 * Implements of hook_block_view_alter().
 */
function fb_comment_notification_block_view_alter(&$data, $block) {
  $flag = variable_get('fb_comment_notification_admin_notify', 0);
  if ($block->delta == 'facebook-comments' && $flag) {
    $appid = variable_get('facebook_comments_appid', '');
    $data['content'] .= '<script>
      //INITIALIZE THE OBJECTS
    window.fbAsyncInit = function() {
    FB.init({
        appId:  ' . $appid . ',
        status: true,
        cookie: true,
        xfbml:  true,
        oauth: true
    });
      FB.Event.subscribe("comment.create", function(response) {
 
     //Send an email with ajax

      jQuery.ajax({
            type: "POST",
            url: "/send/fb-mail-notify",
            data: "urlofpage="+window.location+"&comment_msg="
              +response.message, 
            success: function(html){
            },
            error: function(html){
            }
      });
      });
  };
		</script>';
  }
}

But the Facebook comments module is providing an extra comment form other than block. If user is commenting through that comment form, no emails will trigger.

You can keep your js in to separate file and can include it using hook_page_build(). Then mail will work for both Facebook comments block and Facebook comment form.

Thanks,
ARUN AK

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello pragna,

Reviewers here have done great work ,, and you followed that so I'll do a review to see if this is a RTBC.

Automated Review

found some error ,, not much @pareview.sh:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpragna2580865git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes:Does not causes] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. automated test showed some errors
2. Implements hook_help().
3. in your .module file you are using filter_xss which allows you to padd some allowed HTML tags which I don't see the case here .. if its not the case you can just use check_plain() in your POST variables.
4. also in your .module file, line 81 .. you can use drupal_add_js() for the JS script you have there maybe.
5. also in your .module file, line using hook_menu the 2nd menu you are using 'access arguments' giving this page the "access content" which is by default granted for anonymous role which is bad I guess! I tried to access it with anonymous role it gave me a blank page .. instead you can use another permission to give access denied!

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

although I see no blockers here so I think this is a RTBC :)

Thank you

I will update your account so you can 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 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.

Thanks go the dedicated reviewer(s) as well.