Share buttons for Drupal including the E-MAILiT sharing button, Facebook, Twitter, Pinterest, WhatsApp, Viber, Skype, Telegram, and many more.

E-MAILiT is a powerful social sharing platform and an awesome marketing web tool for your website. You can find out much more in our website https://www.e-mailit.com

project page: https://www.drupal.org/sandbox/e-mailit/2667034

GIT COMMAND FOR CLONE

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/E-MAILiT/2667034.git e_mailit_sharing_buttons

Manual reviews of other projects

Files: 
CommentFileSizeAuthor
#29 js-error.png44.11 KBvisabhishek
#27 coder-results.txt96.18 KBklausi

Comments

E-MAILiT created an issue. See original summary.

PA robot’s picture

Issue summary: View changes
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxE-MAILiT2667034git

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.

Dave Bagler’s picture

Looked through the code here's my thoughts:

Please add a README.txt or README.md file.

The optional files property of the info file is for autoloading class and interface declarations. The three referenced files (emailit.admin.inc, emailit.install and emailit.module) don't include class or interface declarations so I recommend removing the files[] lines from the info file.

The install hook is empty so I recommend removing it.

You set a lot of variables with the administration form. Each of the set variables should be deleted in your module's uninstall hook.

E-MAILiT’s picture

Status: Needs work » Needs review

Thank you very much for the suggestions.
We've made all the changes.

Please review the project.

Thank you in advance.

Arkener’s picture

Status: Needs review » Needs work
Issue tags: +PAReview: security

AUTOMATED REVIEW:
Please take a look at the automated review as the bot has stated: http://pareview.sh/pareview/httpgitdrupalorgsandboxe-mailit2667034git

MANUAL REVIEW:
emailit.admin.inc
Lines 12-16
Use #attached instead of drupal_add_js and drupal_add_css

emailit.module
Line 46: Replace the magic number 1 by NODE_PUBLISHED
Line 111-208: This entire function is full of XSS exploits, you should sanitize each open variable get that is loaded from an open input field, for example: Add the following code to the TWEET VIA (your Twitter username) (emailit_TwitterID) field:
'}; alert('XSS'); var e_mailit_config = {display_counter:false,TwitterID:'test
Line 243: This is also XSS issue, for example: Add the following code to the Services [ Service Codes ] emailit_default_buttons field:
Facebook,Twitter,Pinterest,LinkedIn,"><script>alert('Test');</script><span class="

For more information about sanitizing, please read https://www.drupal.org/node/28984.

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

E-MAILiT’s picture

Status: Needs work » Needs review

We've made the required fixes.
Please review the project.
Thank you in advance.

E-MAILiT’s picture

Priority: Normal » Major

Please review the project so we can promote it to full.
Thanks in advance.

E-MAILiT’s picture

We have fixed all errors reported by automated review tools.
Please review the project.

sourabhutani’s picture

Automated Review

There is some coding standard errors to fix : http://pareview.sh/pareview/httpgitdrupalorgsandboxe-mailit2667034git

Individual user account
Yes

Duplication
No

Master Branch
Yes. Branch 7.x-1.x

Licensing
Yes. Follow

3rd party assets/code
Yes follow. Do not contains any 3dr party.

README.txt/README.md
Yes

Code long/complex enough for review

Yes. Well documented. Extending views sort, fields and argument.

Secure code
None security issue found. Looks good.

Coding style & Drupal API usage
After a manuel Review, everything seems to be ok. Perhaps, just some recommandations :
-Mixed case function formatting used in emailit.admin.js. Use lower case and _ [style_camel_case]

Module works fine, without errors.

E-MAILiT’s picture

Automated Review
All coding standard erros appear in minified javascript files like jquery.min.js

Coding style & Drupal API usage
Mixed case function formatting used in emailit.admin.js is now fixed.

Please review the project so we can promote it to full.

klausi’s picture

Priority: Major » Normal
Status: Needs review » Needs work
PAReview: 3rd party code
jquery.min.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Did you mean to depend on jquery_update module or why is jquery included?

E-MAILiT’s picture

Status: Needs work » Needs review

Drupal 7 already includes an older version of jquery but our module needs a newer one to work properly.
In https://www.drupal.org/node/1058168 describes how we can add a newer version of jquery.
In the code we have added jQuery.noConflict() method to prevent any possible jquery conflict.

E-MAILiT’s picture

We' ve removed jQuery 3rd party libraries.
Please review the project.

E-MAILiT’s picture

Priority: Normal » Major

Please review the project so we can promote it to full.

E-MAILiT’s picture

We' ve made all the requested modifications.
Please review the project.

ziomizar’s picture

Hi E-MAILiT,

In order to speed up the review process, you could be interested in read how to get the review bonus.

Please read https://www.drupal.org/node/1975228.

Be patient, someone will review your code.

E-MAILiT’s picture

Manual reviews of other projects
Queued for review by site administrators. Will be published after approval.

E-MAILiT’s picture

It is almost 5 months since we firstly requested a review!

Please review the project so we can promote it to full.

E-MAILiT’s picture

Issue summary: View changes
E-MAILiT’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
ARUN AK’s picture

It seems like you are using minified version of js(emailit.admin.js) in your module. Please provide normal js file, it would be easy go through your script.

E-MAILiT’s picture

Thanks for the comment,
emailit.admin.js is now provided in normal format.

3ssom’s picture

Status: Needs review » Needs work

Hello E-MAILiT,

Reviewers here have done great work ,, and you followed that so I'll complete from where they stopped!

Automated Review

you still have some errors ..
https://git.drupal.org/sandbox/E-MAILiT/2667034.git

Manual Review

Individual user account
[Yes: Follows] the guidelines for individual user accounts.
No duplication
[Yes:Causes] module duplication and/or fragmentation.
Master Branch
[Yes: Follows] the guidelines for master branch.

but there is warning msg you should see:
Git errors:
The last commit message is just one word, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287

Licensing
[Yes: Follows] the licensing requirements.
3rd party assets/code
[Yes: Follows] the guidelines for 3rd party assets/code.
README.txt/README.md
[No: Does not follow] 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
[No:Does not meet 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. (*) going through notes made in #5 can you explain why variable_get("emailit_follow_services") isn't sanitized like the rest in .module because I didn't find also where its showing to test the result of executing XSS I did? this should be addressed or explained so there will not be a secure code problem .. and this goes til line 208.
  2. I don't really see the deference between you project and other share project? for example ShareThis duplication is bad and Drupal community prefers joining force over competition but its NOT a blocker and as far as the applicant knows about it I'd just mention this is the project page as Similar Projects explaining the deference between the two projects!
  3. in you Implementation of hook_uninstall .. looks like alot of manual variables deletion which can be preformed better by something like this:
    function hook_uninstall() {
      // Delete all vars in E-MAILiT Share Buttons module.
      $sql = 'SELECT name FROM {variable} WHERE name LIKE :name';
      $result = db_query($sql, array(':name' => db_like('emailit_') . '%'));
      foreach ($result as $var) {
        variable_del($var->name);
      }
    
  4. if you removed the 3rd party libraries ,, so you went on dependence? I don't see anything in you .info file for module dependence .. or its not required in your project?

Thank you

E-MAILiT’s picture

Status: Needs work » Needs review

Hi 3ssom,
thanks for the review.

All issues have been addressed,
Please review the project.

3ssom’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

Hello E-MAILiT,

I can confirm all the points were covered and fixed!

FYI: you could have just use check_plain() instead of filter_xss() in your file .module line.. I don't see any allowed HTML tags needed here!

I think this is a RTBC!

klausi’s picture

Issue summary: View changes
klausi’s picture

Assigned: Unassigned » visabhishek
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAReview: review bonus
FileSize
96.18 KB

Review of the 7.x-1.x branch (commit 5e3b0b0):

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. You have to get a review bonus to get a review from me.

manual review:

  1. Do not commit minified CSS files. Drupal will minify them automatically when CSS aggregation is enabled.
  2. Your admin JS file is not formatted to Drupal coding standards at all - it looks like some obscured de-minified version? Can you commit the real source of that emailit.admin.js?
  3. emailit.admin.js: do not use jQuery(document).ready(), use Drupal behaviors. See https://www.drupal.org/node/756722#behaviors
  4. emailit_create_script(): it looks like you trying to pass down settings from PHP to JS here. That should be done with Drupal.settings, see https://www.drupal.org/node/756722#settings-javascript

There is a security vulnerability in this module and as part of our git admin training I'm assigning this to visabhishek so that he can take a look. If he does not find anything within a week I'm going to post the vulnerability details.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

visabhishek’s picture

Issue summary: View changes
visabhishek’s picture

FileSize
44.11 KB

@klausi : Thanks for this assignment

I have manually reviewed the code and my findings are :

use chack_plain instead of strip_tags in emailit_create_buttons()
Ex :

strip_tags($title)

$output_value .= "<div class=\"e-mailit_btn_$stand_alone_button\"></div>" . PHP_EOL;

Use check_url() for $shared_url variable for
$output_value = "<div class=\"e-mailit_toolbox $style \" $shared_url $shared_title>" . PHP_EOL;

Also use proper concatination

$output_value = "<div class=\"e-mailit_toolbox $style \" $shared_url $shared_title>" . PHP_EOL;
AS
$output_value = "<div class=\"e-mailit_toolbox " . $style . " \" . $shared_url . $shared_title . ">" . PHP_EOL;

AND

  foreach ($stand_alone_buttons as $stand_alone_button) {
    $output_value .= "<div class=\"e-mailit_btn_$stand_alone_button\"></div>" . PHP_EOL;
  }


AS

  foreach ($stand_alone_buttons as $stand_alone_button) {
    $output_value .= "<div class=\"e-mailit_btn_" . $stand_alone_button . "\"></div>" . PHP_EOL;
  }

All user facing text should pass through t() function.

See : $node->title in
function emailit_create_node_buttons($node)

I am getting JS error also, Please see the attachement.

klausi’s picture

Assigned: visabhishek » Unassigned

@visabhishek: good find, but how would you exploit the code with a malicious node title?

Here is what I came up with: a' onmouseover='alert(0)

This will only show a nasty javascript popup, but you can come up with more creative ideas to do harm on a site. One more attack idea: a' onmouseover='window.location="http://example.com". Wow, this will redirect you away from the site, perfect for phishing!

When we think about XSS it is important to understand the context in which the user provided string is printed to HTML. In this case the node title is inserted into an HTML attribute. We need to be very careful about that, because strip_tags() will not help us here. We need to make sure that an attacker cannot use quotes to break out of the attribute value for example.

So the solution in this case is to use check_plain(), which will take care of the quotes for you. That way an attacker cannot break out of the context of one HTML attribute and create a completely new one.

visabhishek’s picture

@klausi : Thanks a lot for the valuable suggestion. I will definelty keep in mind the points that you have mentioned during my next security review.

PA robot’s picture

Status: Needs work » Closed (won't fix)

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.

E-MAILiT’s picture

Status: Closed (won't fix) » Needs review

All known problems are fixed.
Please review the project.

sandip27’s picture

Status: Needs review » Needs work

Hello @E-MAILiT

I just did the automated review of project. You can check the output at pareview Review.

root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 5, col 2, Error - Use the function form of 'use strict'. (strict)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 6, col 5, Error - Expected indentation of 2 spaces but found 4. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 7, col 9, Error - Expected indentation of 6 spaces but found 8. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 8, col 13, Error - Expected indentation of 10 spaces but found 12. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 8, col 30, Error - Strings must use singlequote. (quotes)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 9, col 17, Error - Expected indentation of 14 spaces but found 16. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 10, col 17, Error - Expected indentation of 14 spaces but found 16. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 11, col 17, Error - Expected indentation of 14 spaces but found 16. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 12, col 21, Error - Expected indentation of 18 spaces but found 20. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 13, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 14, col 28, Error - Closing curly brace appears on the same line as the subsequent block. (brace-style)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 15, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 15, col 46, Warning - Too many nested callbacks (4). Maximum allowed is 3. (max-nested-callbacks)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 16, col 29, Error - Expected indentation of 26 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 18, col 28, Error - Closing curly brace appears on the same line as the subsequent block. (brace-style)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 19, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 21, col 45, Error - Shadowing of global property 'undefined'. (no-shadow-restricted-names)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 21, col 45, Error - Unexpected use of undefined. (no-undefined)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 22, col 21, Error - Expected indentation of 18 spaces but found 20. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 22, col 21, Error - Strings must use singlequote. (quotes)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 22, col 33, Error - Missing semicolon. (semi)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 24, col 21, Error - Expected indentation of 18 spaces but found 20. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 24, col 21, Error - Split 'var' declarations into multiple statements. (one-var)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 31, col 109, Error - Expected indentation of 22 spaces but found 108. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 32, col 21, Error - Expected indentation of 22 spaces but found 20. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 33, col 29, Error - Expected indentation of 22 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 34, col 29, Error - Expected indentation of 22 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 35, col 29, Error - Expected indentation of 22 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 36, col 29, Error - Expected indentation of 22 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 38, col 93, Error - Expected indentation of 22 spaces but found 92. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 40, col 1, Error - Trailing spaces not allowed. (no-trailing-spaces)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 41, col 21, Error - Expected indentation of 22 spaces but found 20. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 42, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 43, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 44, col 29, Error - Expected indentation of 26 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 46, col 133, Error - Expected indentation of 26 spaces but found 132. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 47, col 29, Error - Expected indentation of 26 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 48, col 29, Error - Expected indentation of 26 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 50, col 1, Error - Trailing spaces not allowed. (no-trailing-spaces)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 52, col 1, Error - Trailing spaces not allowed. (no-trailing-spaces)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 54, col 129, Error - Expected indentation of 24 spaces but found 128. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 55, col 25, Error - Expected indentation of 22 spaces but found 24. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 57, col 29, Error - Expected indentation of 22 spaces but found 28. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 58, col 33, Error - Split 'var' declarations into multiple statements. (one-var)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 58, col 33, Error - Expected indentation of 30 spaces but found 32. (indent)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 58, col 37, Error - 'importColor' is defined but never used. (no-unused-vars)
/root/repos/pareviewsh/pareview_temp/js/emailit.admin.js: line 59, col 41, Error - Expected indentation of 34 spaces but found 40. (indent)

and many more...

Please fix those issue and re-submit for review.

Thanks

PA robot’s picture

Status: Needs work » Closed (won't fix)

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.

E-MAILiT’s picture

Priority: Normal » Major
Status: Closed (won't fix) » Needs review

I've made as many fixes as I could about issues presented in automated review.
There are only minor formatting issues in emailit.admin.js
If there is any automated way to fix them please inform me.
I use netbeans IDE to auto-format the javascript file.
It is almost 1 year I am trying to promote this project to full.
I do not think that formatting is such a big deal to prevent this project from being promoted.
The most of the full projects published do not meet even 50% of the requirements.

Thanks in advance.