[D7] Limit Visit

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

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.

Validated that comment #2 fix is accurate. Testing on stable d7 and saw the same error on Pareview.sh.

Aside from doc comment, Both Coder (Code Sniffer) and Pareview.sh come back clean.

- Module appears to work as expected, but only for logged in users. If public post, did not honor restrictions set by module. Based on use case provided above, this is likely not an important feature, but wanted to note it anyways.

- Need to t() value on line 42 - limitvisit.admin.inc
- Need to t() titles / descriptions in limitvisit.module

Aside from those small house-keeping fixes, I think you're good.

Like Roost.Me I also found that module is taking all anonymous users as same because you are using uid to count user visits and uid for all anonymous users is 0. For this I think you should maintain visit count for anonymous user with some random value and store that random value in session or cookie so that they are separate for all anonymous users.

One more thing you module could be more useful if you can add functionality to assign role to particular rule, so that multiple rules can be add for same url with different roles.

Other than this your module looks good.

Unfortunately, Roost.me gave you some bad advice about adding the t() function to "titles / descriptions".

Manual review

You should not be using t() in the hook_menu() title and description declarations. Menu automatically pushes title and description through t(), you'll end up with things being double-translated. See: function hook_menu in the API documentation.

Manual review

Sorry, but I felt I had to do a more thorough review of this one, and the result of my second code walk-through is that it needs some work.

Automated Review

PAReview reported one issue (IMHO, this is a false positive).

Manual review

Individual user account

Yes: Applicant geekygnr follows the guidelines for individual user accounts.

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation.

Code long/complex enough for review

Yes, about 400 lines. Follows the guidelines for project length and complexity.

Secure code

No: Does not follow the guidelines for writing secure code.
In function limitvisit_admin_redirect, the field value is not sanitized. Entering <script>alert('XSS');</script> triggers an immediate XSS.

Coding style & Drupal API usage

My notes:

• Typo in docblock for hook_permission() ("Implements hook_permissions().")
• Even though many modules don't use the second parameter for variable_get(), it is best practice to use it. If you don't provide a default value, there will be an error if the variable requested does not exist.
• (*) Do not call time(). Use REQUEST_TIME instead.
• (*) Use placeholders for variables in a translated string.
  
  Not: $form_state['values']['value'] . ' ' . t('is not a valid path.'), but:
  
  t('@value is not a valid path.', array('@value' => $form_state['values']['value'])).
• (*) Data from limitvisit_admin_form is not validated before they're inserted into the database. Putting "X" in the field for hours triggers an PDOexception.
• (*) Not all user data is sanitized (re: security review above).

The starred items (*) are fairly big issues and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

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

It comes from the "is not a valid path" error in the message area (note that the part of the message that should have echoed the path is missing).

To reproduce, put the CSS exploit in the textfield and press "Save Redirect" (as shown below):

Using the correct placeholder for variables in a translated string fixes it. Do not have:

$form_state['values']['value'] . ' ' . t('is not a valid path.')

But instead:

t('@value is not a valid path.',
  array('@value' => $form_state['values']['value']))

Your code is still not secure:

Insecure (your code):

form_set_error('value', t('!path is not a valid path.',
  array('!path' => $form_state['values']['value'])));

Secure:

form_set_error('value', t('@path is not a valid path.',
  array('@path' => $form_state['values']['value'])));

Please read Altered the behaviour of placeholders in t() calls and notice that placeholders starting with "!" are not sanitized, but inserted as-is. Only placeholders starting with "@" and "%" are sanitized.

The rest is OK.

I don't think anything below blocks this from being promoted - it's mostly "would be nice" suggestions. They could probably be forked into new issues in the project queue. Everything else looks totally fine to me, so +1 on promoting this.

In README.txt:

To avoid someone from signing into the directory and scraping it for everyone's personal information.

This isn't a complete sentence - how about "For example, this can be used to help limit simple scraping of profile information from users with public profiles."

drupal

It's "Drupal" throughout the file :)

The paht you enter can

Typo here.

In limitvisit.admin.inc:

'#value' => t('Save Redirect'),

Drupal uses sentence case for buttons - so "Save redirect". There's probably a few other places for this too.

In limitvisit_admin_redirect_validate() - you could simplify that a little by using an #element_validate callback on the text field. Then you can use form_error() directly. What you have is still perfectly functional though.

In limitvisit.module:

In limitvisit_boot() - you could probably use the caching API to save on a DB call to load rules, especially as you can easily clear / reset it when you save the admin form.

You might be able to simplify some of this by using the Flood API - https://api.drupal.org/api/drupal/includes!common.inc/function/flood_is_...

@geekygnr, thankyou for your contribution.

Automated Review

PAReview reported some issues. See attachment.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No multiple applications
Yes: No multiple application found.
No duplication
Yes: Not found any similar project.
Master Branch
Yes: Follows the guidelines for master branch.
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt
Yes.
Code long/complex enough for review
Yes. Follows the guidelines for project length and complexity.
Secure code
Yes.
Coding style & Drupal API usage
1. You are opening up your README.txt file in hook_help() that's incorrect. I would personally recommend you, just give specific information about your module inside hook_help().
2. In following if statement, this code && !empty($element['#value']) can be removed because drupal_valid_path() take care of this. Also you can use #required => TRUE property with your form element.

if (!drupal_valid_path($element['#value']) && !drupal_lookup_path('source', $element['#value']) && !empty($element['#value'])) {

3. IMO, both field should be mandatory in limitvisit_admin_form() form. Also Allowed visits per hour field contains validation for numeric value, it should be required. Because If it accept blank value then it contradict with your validation.

As above point looks blocker to me so I am moving this to "NEED WORK".

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

er.pushpinderrana wrote:

You are opening up your README.txt file in hook_help() that's incorrect.

@er.pushpinderrana, why are you saying that? Can you refer to a documentation guideline stating that showing the README.txt file in hook_help() is incorrect?

Here is a link to the text about hook_help() in the official Module documentation guidelines. There is even an example showing how to display README.txt or README.md in hook_help().

@gisle, thank you for correcting me.

Automated Review

There is one error, but from comment #2 and #3, it's a false positive so you are good to go on the automated review front.

Manual Review

In your .install file, you define schema for storing data, but don't get rid of that schema when uninstalling the module. I would suggest adding drupal_uninstall_schema('limitvisit'); to hook_uninstall

Other than that I didn't see any issues as I went through and reviewed your code. I'm going to set this to RTBC as I don't feel the lack of the uninstall_schema warrants holding up the review process.

Ah you are correct! I wasn't aware that it wasn't needed anymore. Thanks for pointing that out!

manual review:

1. limitvisit_boot(): why do you need hook_boot() and cannot use hook_init()? Since your functionality only works for authenticated users this should work with hook_init() as well? Counting the visits for anonymous users is pretty pointless since all of them will be counted under user 0? Please add a comment.
2. limitvisit_boot(): maybe the 3600 seconds interval should be configurable with a variable?
3. Why are you using the HTTP status code 307? Please add a comment.
4. Since you can basically shut down the entire drupal site as admin if you specify a very broad path I would recommend to mark the admin permission as 'restrict access' => TRUE, so that the users know that people can do damage with this permission.

But that are not critical application blockers, so ...

Thanks for your contribution, geekygnr!

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.