[D7] Request statistics

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

This is a good module. But I have found 1 issue of permission.

Automated Review

This module pass the automatic review.

Manual Review

Individual user account

[Yes: Follows / No: Does not follow] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause / No: Causes] module duplication and/or fragmentation.

Master Branch

[Yes: Follows / No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows / No: Does not follow] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows / No: Does not follow] the guidelines for project length and complexity.

Secure code

[Yes: Meets 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. (*) Major finding, needs work
2. (+) Release blocker
3. Just a recommendation
4. ...]

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

Permission issues

line 35 of file "request_statistics_record_menu.php", access callback is always TRUE, so any user can see the record statistics admin page without permission. I think you need to add access control for that, also for all views, becuase your views also not set access control yet.

<?php
public function menu() {
  $items = array();
  $items[$this->rsr->moduleName()] = array(
    'title' => $this->rsr->displayName(),
    'page callback' => $this->rsr->pageCallbackName(),
    'access callback' => TRUE, // Here
    'type' => MENU_CALLBACK,
  );
  return $items;
}
?>

Suggestion

Create a folder for each sub-modules, so modules are able to clearly read and well-structured.

Good catch, this seems to be a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi Peterx,

What i mean is all users can see admin/reports/request-statistics & admin/reports/request-statistics/* pages without any permission. but that reports page will be display statistics information which include user id, browser info, etc,.

Hi Peterx,

I have test again, permission worked well.

Hi Peter,
I checked your code at commit 71edabf7b884462249751e81dd26942dc09312ea, and it indeed contained the security issue mentioned #2. I can also see that you have fixed it in the latest version.
Please note that `none` access setting in View means that there is no access restriction. It does not mean nobody can access the view. I have often found myself forgetting to set access too.

I too can confirm there was a security issue in the module, so I think we should leave the Security tag. Again, don't worry about the tag, They are purely for stats and educational. There are many modules which had this tag and yet they passed the review.

This PAReview: Security tag is only for educational and statistical purposes. It doesn't mean the current version of the project contains any security issues, and it's certainly not a blocker once fixed.

Manual review

- Package name of the module suit would make more sense if use "Request statistics" (instead of "Request").

- Since this module serializes the user input data (RequestStatisticsRecordPageCallback::pageCallback), note that you should be careful when unserializing them. I did not see the module unserializing them itself, but if you plan to do, please try to avoid Object Injection attacks.

- In the request recording JS file (request_statistics_record.js), the script always assumes the site is run in the top level folder. I had my test site in a sub folder, and I did not get any stats because the JS file pushed data to a different URL. See the attached patch for a fix. Always use Drupal.settings.basePath to get the base path. It will be either "/" or the path to the sub folder, so you can safely assume this works any site setup regardless of the installed location. I will set this application to Needs Work because of this.

- Consider adding some conditional reporting mechanism to prevent repeated submissions. .resize() event can be called thousands of times, and this snippet can this trigger loads of requests to the server. A time delay would do the trick.

I will use the module for some to gather some statistics in the database, and provide another review if necessary.

Hi Peter,
The security issue raised a few days has nothing to do with module's progression. It merely works as a bookmark for others to learn from it. I do not want to take this any further, but please note @everright mentioned about the unprotected Views pages. It's perfectly OK to leave the Ajax path unprotected. Even the Drupal core statistics module does that. But leaving site information available to others by default is definitely an issue.

Take the "Request statistics path" and "Request statistics visitor by visitor" views for example. It's known that Drupal user IDs are unsigned integers. But the exact user IDs cannot be determined by anonymous users if they don't have permission to access user profiles. They can of course tinker with URLs to check 403 vs 404 statuses, but that is not a reason to leave them available in plain sight.

Code shown in Everright's #2 comment is not exactly where the security is. rsr->menu()'s router item does not need protection, but RequestStatisticsViewMenu->menu()'s menu router item does.

- In request_statistics_view_reports.php file, the t("@text") can cause problems in translation because it has no string literal in it. You can use check_plain($display_options['display_description']) to achieve the same.

I'll leave for you or another senior git administrator to decide if the Security tag should stay. I vote to keep it. Nonetheless, now that it is fixed along with other known bugs so far, I'm changing status to RTBC.

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

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /Users/matt/PAR/pareviewsh/pareview_temp/README.txt
  ----------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ----------------------------------------------------------------------
   160 | ERROR | [x] Expected 1 newline at end of file; 0 found
  ----------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ----------------------------------------------------------------------
  
  Time: 3.28 secs; Memory: 22.25Mb
  

• 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

Secure code

(*) If I am reading this right (see comment below about OO usage), the page callback in the request_statistics_record module
has open access. This takes POST data, and writes to the database w/o a token check. An attacker could insert bogus data int
the tables, update any row, and could also write a curl script that fills up the database (ie, a DOS). You need to look
into drupal_get_token() and drupal_valid_token(). I am deeming this a security problem that needs to be fixed.

(*) In RequestStatisticsViewMenu::menu(), the permission 'access administration pages' is too loose. There is potential
for information disclosure about uid 1 habits. Define your own permission, or use 'administer site configuration'.

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:
Your .info file should use the files[] directive for the includes so you can leverage the autoloader instead of manually including files.

Schema should list of FK for documentation purposes, and FK column names should really match up with the name in the foreign table (eg, uid). I suspect your FKs should also be indexed, as then will be used with joins.

request_statistics() should use drupal_static instead of the PHP static.

The JS should use behaviors instead of .ready(). Some minor formatting problems, too.

Not a huge fan of the way OO got implemented here. It breaks the pattern of Drupal conventions, and makes this a lot harder to trace out.

In general #attached is preferred over drupal_add_js(). Since you are using this from a hook_init(), it will get called for 404s, image deritive creation, and other things that are not direct page views.

Why do you define a view and then manually call it in a page callback? Just add a Page display to the view and set a path.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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

This review uses the Project Application Review Template.

@Ayesh, when you set RTBC on a module, you can assign it to one of the other admins for a second look. Typically, we don't bug @klausi, except for second opinions on security related issues, but you can pick anyone who is active. That way, it will show up in our queues, and we can get to them quicker.

Thanks Matthew. You are right I should have been careful. Sorry.

However, for the security issue, Drupal core statistics module does a similar thing to add a row.
Please see http://cgit.drupalcode.org/drupal/tree/modules/statistics/statistics.php...

I'm completely agreeing with the hook_init though. It's not in D8, and majority of D7 modules are using it already.

OK, checked with @klausi on this one.

The open path w/o a token check is not a blocker.

The permission of the view is b/c it is potential information disclosure. I will approve the application if the view is changed to use a custom permission or 'administer site configuration'.

I will look at this tomorrow.

Automated Review

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

• 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

I read `git diff b027e11`, and saw several of my recommendations were implemented, and my security concern was addressed. As the security concern was the only blocker, this is back to RTBC.

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

This review uses the Project Application Review Template.

Thanks for your contribution, peterx!

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.

Nice work Peter. That's a module I'll be using. Congrats!