[D7] Spamfree Email

Hi abhiklpm,

Apologies, this is not a review, but I just wanted to chip in to say this looks like a great module.

I had a quick look through how it all works and I didn't see anything jumping out at me, but since I am on the road at the moment, I don't have the time to download and install it to check it out.

Hopefully it'll get through the application process quite quickly as it genuinely looks like a very interesting piece of work. Well done to you.

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 abhiklpm,

Just had a quick look at your manual reviews. I think they are a bit short to qualify. You really need to show that you took the time to install and investigate the modules and offer a full review of them. A single comment is generally not enough.

There is documentation about what these manual reviews should entail. Make sure you have a read of it, it's quite helpful.

The module works great.

Automated Review

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

Manual Review

Individual user account
Yes

No duplication
Yes

Master Branch
Yes

Licensing
Yes

3rd party assets/code
Yes

README.txt/README.md
No (README template)

Code long/complex enough for review
Yes

Secure code
Yes

Nice module!
There was a similar module that promised to do the very same, but unfortunately it had some security issues (which apparently reported by myself), and there were no fixes for that.

I installed the module, and works as it should.

- Module only obfuscates the first item in a multi-field array. Adding a foreach block to handle all field items should do the job (.module:48 of 684fd8f3)

- When the no-js fallback is enabled, it become pretty obvious and easy to extract all the email addresses easily.

$html = file_get_contents('http://dev/work/applications/spamfree/node/1');
$html = str_replace('%40', '@', $html);
$pattern = '/[a-z0-9_\-\+]+@[a-z0-9\-]+\.([a-z]{2,3})(?:\.[a-z]{2})?/i';
preg_match_all($pattern, $html , $matches);
dpm($matches[0]);

This fetches the file, and the noscript tag contains the raw email address.

I can imagine two methods to solve this (other than dropping the 2% of js-disabled browsers).
- Pass the entity type, entity ID, field name, field item index and the occurrence position as URL arguments, and get the module to convert that value to the email-image.
Note that unless you check entity access, this can be a severe security issue. See SA-CONTRIB-2013-011.

- Use symmetric encryption to encrypt the email, and get server to decrypt and send the image back. In this case make sure you add some sort of DoS protection. This can be as simple as a drupal_get/validate_token call. Session independent tokens, the better. See SA-CORE-2013-002.

I'm sorry but this introduces a security issue with the encryption key and anti-DoS security key.

1. The encryption key is not unique across the installed sites. The key is always !^@#&*$%, so an attacker could easily use this to decrypt the email address back.

Solution: You can use site-specific keys to encrypt email addresses. Since you do not store them in the site, drupal_get_private_key() can be used as the symmetric key.
Also, I would add some function_exists() calls to make sure this function exists, because in some server configurations, mcrypt module is not available. We better add this in a hook_requirements hook too I think.

2. The drupal_get_token() call uses the string literal "date". While this token is unique across sites and sessions in a single site, same token is used for a site across a single session, which bots can be programmed to parse ones and requires a plethora of images, which will bypass the protection.

Consider passing the encrypted string to generate the token. This can harden the protection. If you check the usages of drupal_get_token in other modules and core, you will see the main variable is passed as the argument.

I'm sorry I had to add the security tag to this. Please do not remove this tag because it helps others to learn from these applications. This module will be very helpful to many people (myself included), so I will continue to keep this application live and give any help if needed.

Awesome work!
No blockers I could find in a manual review, but a few performance improvements.

In both encryot and decrypts, module calls the (de/en)crypt functions multiple types. You can use the $_GET variable again and again without encrypting it again.

Also, you won't need to define the encryption key in a PHP constant. It's always unique even if you call it multiple times.

This module looks pretty much RTBC otherwise to me, but let's wait for a few others to review this. As always, taking the Review Bonus will speed up the process.

I was not aware of this, but there seems to be a module Invisimail, which does the same more or less. Since that module already has 14k installs, I think people will want the noscript improvements to that module.

I'm not a git admin here, but I think you can still get vetted status on drupal.org and become a maintainer of that module (The project page says it's looking for new co-maintainers).

Sorry for not being able to catch this up.

I would remove the define("SPAMFREE_EMAIL_ENCRYPTION_KEY", drupal_get_private_key()); and use the drupal_get_private_key() directly because we have one less constant to keep in memory and the drupal_get_private_key() is cached anyways.

Otherwise looks good to me.
Consider taking a review bonus (make three manual reviews of other projects and link to them in your application, and add the PAReview: review bonus tag).

I'm not a git admin here. Just volunteering to maintain the queue. If we can get a few others to review this projects, this can be marked "Reviewed and tested by community". Klausi and other git admins will take a final look at this and then process this. Good luck!

Manual Review

Individual user account

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

No duplication

[Yes: Causes] module duplication and/or fragmentation.

Please compare with Invisimail

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 the guidelines for in-project documentation.

Code long/complex enough for review

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

Secure code

I haven't done any security review

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

Removing review links that are not manual reviews.

manual review:

1. project page: what are the differences to existing modules such as https://www.drupal.org/project/spamspan and https://www.drupal.org/project/geo_filter and https://www.drupal.org/project/invisimail and possibly others? Please list the existing modules on the project page and describe the differences to them.
2. "alt='loading'": all user facing text must run through t() for translation. Same for "Non JS Enabled browser settings" in the *admin.inc file.
3. spamfree_email_menu(): doc block is wrong, the first line should be just "Implements hook_menu().". See https://www.drupal.org/coding-standards/docs#hookimpl
4. arial.ttf appears to be a proprietary font according to https://en.wikipedia.org/wiki/Arial . Since everything hosted in git repositories on drupal.org must be GPLv2+ this font should be removed. Instead you should tell users where to download it on the project page and the README.
5. "form_set_error(t('spamfree_email_nojs', "PHP GD library is NOT installed on your web server"));": you are using t() wrong here, the second argument should be an array. Did you mean to use the second argument as first argument?

The licensing issue is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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.

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/spamfree_email.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   33 | WARNING | Open page callback found, please add a comment before the
      |         | line why there is no access restriction
  ---------------------------------------------------------------------------
  

• 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. spamfree_email_process_email(): why do you call drupal_session_start() here? Does that mean your module breakes page caching for all anonymous users? I think that call should just be removed - it is fine to have the same token for all anonymous users.
2. spamfree_email_text_2_image(): why do you call spamfree_email_encrypt() here? Just use the already encrypted variable in $_GET['text']?

Otherwise looks good to me.

Assigning to naveenvalecha as he might have time to take a final look at this.

I think Klausi meant that using the session makes Drupal not to cache the pages, which is actually a big deal.
drupal_get_token/drupal_valid_token functions take the user session because the variables used to generate the token are known. A path, or something else that the user can figure out. Since each user has different sessions, this makes sure the user who generated the token is same as the user who validates it.

I apologize that it's I who mentioned this in first place.

This can be as simple as a drupal_get/validate_token call. Session independent tokens, the better. See SA-CORE-2013-002

But again, session independent tokens, the better.

I'm currently on mobile and was checking my tracker on the road. I will clone your repo again and write another review on latest status.

no objection for a week, so ...

Thanks for your contribution, Abhilash!

I updated 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!

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.