[D7] E-MAILiT Share Buttons

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.

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.

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.

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.

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?

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.

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.

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

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!

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

@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.

@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.

@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.

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 @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

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.

Thank you for your contribution!

After 2017 March 7 everyone can promote a project to a full project.  A full project has a short project name and a drupal.org/project URL.  It can also have releases (like alpha1 or 1.0).  Edit your sandbox project, and then choose the 'Promote' tab.

https://www.drupal.org/docs/8/understanding-drupal-version-numbers/drupa...
https://www.drupal.org/docs/8/choosing-a-drupal-version/what-do-version-...
https://www.drupal.org/docs/8/understanding-drupal-version-numbers/what-...
https://www.drupal.org/docs/8/choosing-a-drupal-version/release-stable-v...

If you'd like to opt into security coverage, please ensure your module is ready for a full release, and then set this issue back to 'needs review'

Immense apologies for how long it took to get to this review completed.

I am closing this application for the lack for replies. I take the OP just needed to be able to promote the project.