[D7] Cron Interval Actions

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

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.

Automated Review

[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.]

Manual Review

Individual user account

Yes Follows the guidelines for individual user accounts.

Secure code

[ Yes: List of security issues identified.]

• Configuration form Accepts malicious code like " < script >alert(1);< / script > " and then this script runs too. Screenshot attached.

Coding style & Drupal API usage

List of identified issues:

1. *Security Issue at configuration form
2. In hook_uninstall no need to uninstall cron_interval_actions schema manually, Drupal itself will uninstall all those schemas registered during installation.

Recommendations for Application Issue

For title of the issue use: [Dx] Your project name

Provide Similar Modules and how your module is different from others.

Adding the tag as per #4.

Automated Review

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/cron_interval_actions.module
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
340 | ERROR | [x] Whitespace found at end of line

Pretty minor, and shouldn't stop acceptance.

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation. While I think that other modules provide the same functionality as this one does - they also provide a wide range of other functionality that I can imagine is not always needed by people who need this functionality. I can see myself using this module, but discounting the others as too heavyweight.

Master Branch

Follows the guidelines for master branch.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

A README.md is provided which is useful and covers many of the points of interest. The code example at the end of the document should be marked up as pre-formatted so that formatting is preserved when rendered by a markdown renderer.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

Meets the security requirements.

Coding style & Drupal API usage

Nothing major. I'd consider the first two as needing fixing though, the rest are just suggestions.

• Interval names are double encoded on the admin page (Try entering an interval called "Every day > 9" - the > will should as > on the from on the admin page.)
• The following should all end in a "y", not "i" (From cron_interval_actions_fields()) : Januari, Februari, Juli
• Add configuration link to .info file so that configuration can be accessed from the module list in Drupal.
• In cron_interval_actions_cron() if intervals aren't enabled you could exit straight away and then you have less indentation and easy to read code, e.g.
  

  if (!variable_get('cron_interval_actions_enabled', FALSE)) {
    return;
  }
  

Hi Patrick,
I have not tested the module fully yet, but to mention a few points;
- In the menu callback for admin/config/system/cron_interval_actions/test_run/%, since it only has an access check, this menu router is vulnerable to cross site request forgery.

For example, suppose someone tricks an administrator with administer cia settings permission to open a URL that would run the cron. <img src="http://example.com/admin/config/system/cron_interval_actions/test_run/5">.

Administrator is not aware of this, but if the administrator is logged in to example.com, he would have run the cron with id 5.

You can overcome this by adding a token to the URL and validate it before running the cron test run. drupal_get_token() and drupal_valid_token() make that dead easy.

- Also, please do not use drupal_goto() inside a form_submit function. This will prevent Drupal from executing the full form submit (allow form_altered additional submit handlers from running) because drupal_goto() calls die() when called. Setting the destination to $form_state['redirect'] will do the trick.

Good Job Patrick.

Used it today in a project. Simple and effective!

Did a manual code review and did not find any issues.
All issues mentioned above are fixed so I'll mark this Reviewed & Tested.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2563595

Project 2: https://www.drupal.org/node/2707655

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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

manual review:

1. "drupal_valid_token(check_plain($params['token']))": the check_plain() is wrong here since you are not printing any data to HTML here. Make sure to read https://www.drupal.org/node/28984 again.
2. cron_interval_actions_save_interval(): the check_plain() is wrong here since you are not printing to HTML here. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984

Those are blockers right now, because you need to know when to use check_plain() and when not.

I have reviewed all the code on security issues, both point from #24 are fixed.
For completeness I included the git diff output for the last changes.

manual review:

1. The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722
2. cron_interval_actions_cron(): whouldn't the watchdog message be logged AFTER you executed the actions? Looks confusing when the log is done beforehand but says that the action has already been executed.

But otherwise looks good to me.

Thanks for your contribution, Patrick!

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.

I am giving credits to the users who participated in this issue.