[D7] DB Track

Hi,

Had a quick look on this. Below is my review...

1) No pareview errors & No Coder Module errors.

2) DB_TRACK_REPORT.ADMIN.INC :
You have listed the theme table which is ok for small data - Say 20 - 30 Entries on page, But in case of large number of entries - the query execution and page load will be more.
This is not good from user perspective and also from system health persepctive.
So, i think you should go for theme("pager") by limiting the number of records on page to minimum value.

3) DB_TRACK_SETTINGS.ADMIN.INC :
You have created a simple textarea for URL / LINE.
But you are directly saving data inside the variable and not validating anything.
For Example: was able to enter this inside the fieldset...

a) "!@#$%^&*&^&((*()JNJKLM:"}".
b) db_query("SELECT nid, title FROM {node}");

I doubt if that should happen from security point of view.

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 kunal.kursija!
I have just reviewed your code and get some points:

1. db_track.install
drupal_uninstall_schema is no longer necessary in Drupal 7 https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_un...

The installation of database schema occurs when the module is first enabled. You may then use hook_install() to perform actions on your database tables. The uninstallation of database schema occurs after your implementation of hook_uninstall(), allowing your module to act on its data.
https://www.drupal.org/update/modules/6/7#install-schema
2. Some spelling mistake:
a. db_track.info
loging => logging in description
b. Exclud in Readme.txt(30)
c. REPOTRING => REPORTING in Readme.txt(33)
d. dependences => dependencies

Hi Andrdrx,
Module seems stable to me. I have tested this and worked fine for me.

This is a good to have a module!!
Kudos to your work.

Hi @andrdrx,
Thanks for contribution.
Some points from my side:

1. hook_permission is missing, you passed the access argument in hook_menu but not used them.
2. Please fix this initial notice (screenshot attached).

Here are a few issues:

1. There is a small typo on the settings form:
"Specify pages by using their paths. Enter one path per line. Forms on this pages would not be tracked."

Should probably read:
"Specify pages by listing their paths, one path per line. Forms on these pages will not be tracked."

2. It is also not clear whether the exclusions by path allows wildcard paths. To keep consistency with other fields like this wildcard paths should probably be supported here, and indicated as such in the description for the field.

3. Additionally, it would be better to include the paths you're hardcoding on line 16 of the .module file in this field, rather than hardcoding them in code. It may be that people want to track changes to forms on those paths, but even if they do not, being able to see them in the list of excluded paths would be beneficial.

So instead, I would move this to an install hook that sets these paths as the default value of the variable.

array_push($excluded_urls,
    'admin/content',
    'admin/modules',
    'admin/people',
    'admin/modules/list/confirm',
    'admin/reports/db_track'
  );

4. Rather than a regular submit handler to drop the db track table, this would be a good instance to use a confirm form.
5. Instead of fetching all results from the database then chunking them for the pager, extending the query with the PagerDefault class would be a nicer solution. Also it would probably be handy to be able to sort the table by clicking headers. Here's a good writeup on implementing a paged table with tablesort: http://w3shaman.com/article/working-table-and-pagination-drupal-7

6. Some of your lines are a bit lengthy, especially some of the if statements with many conditions to be checked, wrapping those onto multiple lines would improve readability a bit.

1. db_track.module line 9: comment should be "Implements hook_form_alter()."
2. db_track.module line 108-109: don't see a reason to use form here. Use simple page callback and instead of db_track_drop_track_table function make 'Drop track table' button as a link to "admin/config/development/db_track/drop_track".
3. db_track.module line 127-128: same here, no need to use drupal_get_form.
4. db_track.module line 138: remove unnecessary code after drupal_goto() that will never execute.
5. Rethink the whole section (items 2-4): unnecessary forms, drop action without confirmation form.
6. db_track_settings.admin.inc line 14: broken line near '*', use double quotes "*".
7. Fix the issues reported by coder: http://pareview.sh/pareview/httpgitdrupalorgsandboxandrdrx2432537git (your link from description ;-))

2. Made simple callback instead form. Is this a good idea to exclude confirm form on Drop Table action?

Sorry for confusing, yes confirm form is required in "admin/config/development/db_track/drop_track" callback.

1. db_track.module: Don't mix hooks and custom functions, put hook functions on the top of the file with hook_menu on the very top.
2. db_track_report.admin.inc: line 13, use l() function and wrap text in t() function.
3. db_track_settings.admin.inc: line 16, 18: unnecessary parameters. By default textfield has '#cols' => 60, and any form element by default has '#required' => FALSE (see Form API).
4. db_track_report.admin.inc: line 62-70. Same unnecessary default parameters ('attributes', 'sticky', 'colgroups'), "caption" => "" will create empty "caption" tag <caption></caption>, do you need it?

1. db_track_settings.admin.inc line 13: remove leading slash in link path.
2. db_track.module line 166: use $form_state['redirect'] instead of drupal_goto(). Form submission handler should never use drupal_goto(). If you use drupal_goto() you will short cut the form processing skipping any other form_submission functions that have attached themselves to the form.

1. db_track.module line 166 | ERROR | Expected 1 space after "="; 2 found
2. Check http://pareview.sh/pareview/httpgitdrupalorgsandboxandrdrx2432537git after each commit. There are two errors now.

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

manual review:

1. db_track_schema(): why is the user column VARCHAR and not INT? Shouldn't this point to user IDs? Please add a comment.
2. db_track_form_alter(): why do you check for the "administer actions" permission here? Please add a comment.
3. db_track_start_field_collector(): what exactly does the function return? Please add @return docs, see https://www.drupal.org/coding-standards/docs#return
4. "db_delete('db_track')->execute();": looks like you want to use db_truncate() instead? See https://api.drupal.org/api/drupal/includes!database!database.inc/functio...
5. This module has a security vulnerability and I'm assigning this to Manjit.Singh as part of our git admin training so that he can take a look. If he does not find the vulnerability I'm going to post details about the security issue in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Thanks Klausi for assigning this issue !!!

@andrdrx db_track_settings_form is vulnerable to XSS exploits. when i enter any alert in /admin/config/development/db_track textarea. I will get a nasty javascript popup in /admin/reports/db_track reports. You need to sanitize user provided text before printing, Please check https://www.drupal.org/node/28984 and https://api.drupal.org/api/drupal/includes%21common.inc/group/sanitization/7

db_track_menu() is also vulnerable, Anybody can access admin/config/development/db_track/drop_track url and delete the track table.
Please check in screenshots, I am anonymous user and easily access this form.

forget to attach screenshots :)

The Git commits are still 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

manual review:

1. db_track_table(): don't call theme() here, just return a render array and pass that on in db_tracker_table(). Drupal core will render it for you later. See https://www.drupal.org/node/930760
2. Storing user names: the problem is that user names can change while the user ID never changes. And you can easily join to the user table when displaying the rows where you can get the user name.
3. db_track_table(): this is still vulnerable to XSS exploits. User names are also considered untrusted user provided text which must be sanitized. While there are restrictions in the registration process which characters a user name can contain they might also get imported from a third party system, so the Drupal security policy is that user names must be escaped.

manual review:

1. db_track_schema(): foreign key to the user table is missing, see https://api.drupal.org/api/drupal/includes!database!schema.inc/group/sch...
2. db_tracker_table(): doc block is not useful, callback function for itself? See https://www.drupal.org/coding-standards/docs#functions on how to document page callbacks.
3. db_tracker_table(): do not call drupal_render() here, just return a nested render array. See https://www.drupal.org/node/930760
4. db_track_table(): instead of calling user_load() fot every single table row I would call user_load_multiple() to only have one query. And for rendering the link to the user you could use theme('username', ...) instead.

But that are not critical application blockers, otherwise looks RTBC to me.

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

1. The file comment is wrong in db_track.install:
   

   /**
    * @file
    * Variable API module install file.
    */

2. One shouldn't assume anything and force hard-coded values in db_track_form_alter(). Hard-coded assumptions are rarely always correct. Instead, set the default value of db_track_excluded_url to include those values.

However, these aren't blockers.

Thanks for your contribution, Roman Yaremiy!

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.