[D7] Taxonomy Comment

1: Found menu callback with 'access callback' => TRUE for all defined menu hooks. Give proper permission for menu callbacks in hook_menu()

$items['admin/content/taxonomy-comment'] = array(
    'title' => t("Taxonomy Comments"),
    'page callback' => 'drupal_get_form',
    'page arguments' => array('comment_listing'),
    'access arguments' => array('comment_listing'),
    'access callback' => TRUE,
    'file' => 'include/taxonomy_comment_include.inc',
    'type' => MENU_LOCAL_TASK,
  );
  // Implementing the menu item for delete a comment for a taxonomy
  $items['taxonomy_comment/%/delete'] = array(
    'title' => t("Delete Comment"),
    'page callback' => 'drupal_get_form',
    'page arguments' => array('taxonomy_comment_del_confirm', 1),
    'access arguments' => array('taxonomy_comment_del_confirm_del'),
    'access callback' => TRUE,
    'file' => 'include/taxonomy_comment_include.inc',
    'type' => MENU_NORMAL_ITEM,
  );
  // Implementing the menu item for edit a comment for a taxonomy
  $items['taxonomy_comment/%/edit'] = array(
    'title' => t('Edit Comment'),
    'page callback' => 'taxonomy_comment_edit',
    'page arguments' => array('comment_listing', 1),
    'access arguments' => array('comment_listing'),
    'access callback' => TRUE,
    'file' => 'include/taxonomy_comment_include.inc',
    'type' => MENU_NORMAL_ITEM,
  );
  // Implementing the menu item for bulk delete to the comments for a taxonomy from listing page
  $items['admin/content/taxonomy-comment/bulkdelete'] = array(
    'page callback' => 'drupal_get_form',
    'page arguments' => array('comment_listing'),
    'access arguments' => array('administer nodes'),
    'access callback' => TRUE,
    'file' => 'include/taxonomy_comment_include.inc',
    'type' => MENU_NORMAL_ITEM,
  );
  // Implementing the menu item for bulk update to the comments for a taxonomy from listing page
  $items['admin/content/taxonomy-comment/bulkupdate'] = array(
    'page callback' => 'drupal_get_form',
    'page arguments' => array('comment_listing'),
    'access arguments' => array('administer nodes'),
    'access callback' => TRUE,
    'file' => 'include/taxonomy_comment_include.inc',
    'type' => MENU_NORMAL_ITEM,
  );
  // Implementing the menu item for vocabulary select for enable or disable comments for that vocabulary
  $items['admin/structure/taxonomy/comment_vocab'] = array(
    'title' => t('Taxonomy Select'),
    'description' => 'Select the Vocabulary',
    'page callback' => 'drupal_get_form',
    'access callback' => TRUE,
    'page arguments' => array('select_vocab_form'),
    'type' => MENU_LOCAL_TASK,
  );

2: XSS Issue : If I enter <script>alert('XSS');</script> in comment and subject, its getting executed.

  $form['taxo_comment']['comment-title'] = array(
    '#type' => 'textfield',
    '#title' => t('Subject'),
    '#required' => TRUE,
    '#default_value' => ($cRow['tc_title']) ? $cRow['tc_title'] : '',
  );
  $form['taxo_comment']['comment-body'] = array(
    '#title' => t('Comment'),
    '#required' => TRUE,
    '#type' => 'text_format',
    '#format' => 'full_html',
    '#default_value' => ($cRow['tc_body']) ? $cRow['tc_body'] : '',
  );
  $form['taxo_comment']['comment_id'] = array(
    '#type' => 'value',
    '#value' => $cRow['t_cid'],
  );

You need to sanitize data before printing. For more information about sanitizing, please read https://www.drupal.org/node/28984.

3: Please remove used variable under hook_uninstall();
example :
variable_get('get_vocab', '');

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.

Removing one item from the issue summary which is not an actual review.

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. "chnages done" is not a useful git commit message. See https://www.drupal.org/node/52287 for your future commits.
2. taxonomy_comment.info: why do you need to add your CSS to all pages that are served by Drupal? Shouldn't you only add it when taxonomy comments are displayed?
3. taxonomy_comment_user_access_admin(): why is only user 1 allowed to access administrative pages? You should provide and use your own permission instead. See hook_permission(). Relying on user 1 is bad because user 1 should never be used on a production site (it has all access rights and is a primary target for attackers).
4. Path /taxonomy_comment/%/delete: any authenticated user can delete any comment? They should only be allowed to delete their own comments, not comments from other users, right? An attacker that has any account on the site can delete all comments! This is currently a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. taxonomy_comment_schema(): why do you save the term name in this table? I think you should only store the term ID? What if the term name changes, then the data is outdated? Same for the user name.
6. _taxonomy_comment_bulkupdate_batch_finished(): why do you need to clear all caches here? Please add a comment. You should only clear the specific caches that need to be deleted, clearing all caches might be an expensive operation an large/busy sites.
7. "$login . t("or") . $signup . t("to post comments"));": do not concatenate translatable strings like that, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7.x
8. taxonomy_comment_theme(): All theme keys should be prefixed with your module name to avoid name clashes with other modules. Use "taxonomy_comments" or similar.
9. taxonomy_comment_form(): instead of using arg() here you should pass the comment ID in with drupal_get_form().
10. "'#default_value' => ($cRow['tc_title']) ? check_plain($cRow['tc_title']) : '',": check_plain() is wrong here since the form API already runs check_plain() for you on #default_value. And double escaping is bad. Make sure to read https://www.drupal.org/node/28984 again.
11. taxonomy_comment_form(): this is vulnerable to XSS exploits. User names are considered to be untrusted user provided data and must be sanitized before printing into HTML #markup. Use placeholders with t() in this case, the "@" placeholder will sanitize it for you.
12. taxonomy_comment_form_submit(): do not use check_plain() on values that are inserted into the database. "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

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Below are my reviews :

[+] Dont use camel case variables.
[+] I am getting error attached the file https://www.drupal.org/files/issues/error_236.png

Removed some automated reviews from the issue summary.

manual review:
why is the menu tab on admin/structure/taxonomy called "taxonomy select"? Shouldn't it be "taxonomy comments" or similar?

The comment deletion security vulnerability still exists. Attack scenario:
1. grant the "Add/show comments" and "Delete Own comment" permission to the authenticated user role
2. Add a victim user account
3. Log in as victim user and create a comment on a taxonomy term. Copy the link the delete the comment (but do not delete the term).
4. Add an attacker user account.
5. Log in as attacker.
6. Paste the copied term deletion link into your browser URL bar. You are able to delete the comment of another user, which is an access bypass security vulnerability.

manual review:

1. taxonomy_comment/%/delete: instead of doing the access check in your page callback you should have a dedicated access callback defined in hook_menu(). That way the access handling is not mixed with the actual page logic.
2. taxonomy_comment_listing(): you are performing one user_load() and one taxonomy_term_load() for every row which might be slow when you have many comments. user_load_multiple() and taxonomy_term_load_multiple() might be be solutions to that. Even better would be Views integration for your database tables then this could be a view.
3. taxonomy_comment_listing(): this is vulnerable to XSS exploits. user names and term names are user provided text and need to be sanitized before printing to HTML tables. Make sure to read https://www.drupal.org/node/28984 again. For the user names #theme => 'username' is a good solution which sanitizes for you.
4. taxonomy_comment_each_term(): do not hard code user roles here, check for permissions instead. Maybe you misunderstood my previous comment? I was just describing an attack scenario, not that you should check the roles of authenticated users. Instead, you should use user_access() to check for permissions of a given user.
5. taxonomy_comment_theme(): doc block is wrong, this is a hook. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
6. "$login . t("or") . $signup . t("to post comments"));": do not concatenate translatable strings like that, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7.x
7. taxonomy_comment_each_term(): do not hard-code access checks to uid 1. user_access() will already make sure that user 1 always has access.

Hi @Satyam Upadhyay

Below are my manual review:

1. Line no 209 and 214 its good practice to use l() function for create link.
2. Do not write query on tpl file use variable and pass in theme() function (taxonomy-comments-tpl)
3. In taxomony-comments-tpl file use taxonomy_comment_userload() function you have already written in inc file instead of user_load() because you have to get only user name.
4. Add hook_help() in your module file.

Thanks for the review @ishwar! Yep, template files must not contain database queries. Data that is required in templates should be prepared beforehand and passed down as variable in hook_theme() or it should be prepared in theme preprocess functions.

Hi @Satyam

Nice extension. Very well, i see your module's file and reviewed by installing on latest D7 version.

One issue I found
when I write comment in comment box and save it, some html tags generated automatically and are not converted properly.

Refer to the attached screenshot.

manual review:

1. taxonomy_comment_op_check(): do not hard code user roles. Use the user_access() function instead of checking user roles. You could define a new permission "administer taxonomy comments" for that.
2. taxonomy_comment_op_check(): where do you check if the user is allowed to edit her own comment? That should be done here, right?
3. taxonomy-comments.tpl.php: do not do access check in templates. That should be done in the access callback. And again, do not load all permissions use user_access() for access checks.
4. t('<h2>Select Vocabulary</h2>'): HTML tags should be outside of t() where possible.
5. taxonomy_comment_form_submit(): the filter_xss_admin() call is wrong 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
6. taxonomy-comments.tpl.php: htmlspecialchars_decode() looks dangerous here. You should not need to sanitize in the template because all of that should be done before variables are passed down to the theme. You also shouldn't call loading functions in your template. Your template should only contain logic to arrange the variables that you have prepared beforehand.

manual review:
"user_access('administrator')": it looks like you have not understood the Drupal permission system. You need to pass a permission name to user_access(), not a role name. Please read up on Drupal roles and permissions and test your code before you set this back to "needs review" next time.

Hi Satyam Upadhyay,

Your module is working fine but I want to ask you as it looks like an error to me.

When I am commenting a taxonomy term, then under "admin/content/taxonomy-comment" the comment is showing more than 1500 times on that page.

Ex: i commented Test. => it is showing more than 1500 in the lists.

Kindly look into this.

Hi Satyam Upadhyay,

I have uploaded the screenshot. Kindly refer to it.

This comment is coming from 1st page to 15th page(i.e. approx 739 posts)

@Satyam Upadhyay

This issue comes when we edit any term and try to add comment.

Suggestion:

1. Remove comment section from edit term page.
2. Add dependencies[] = taxonomy in .info file.

Hi,

in your .module file please check your hook & non hook functions these exceed 80 character limit
https://www.drupal.org/docs/develop/standards/coding-standards#linelength
for example in

function taxonomy_comment_help($path, $arg) {}
$output = t('This module helps to create commenting on the taxonomy terms as per your selected vocabulary.');

thanks

Hi,

Working fine for me now. Great work.

manual review:

1. taxonomy_comment_help(): do not split up sentences over multiple t() calls. One sentence/paragraph should always be one t() call to make sense in other languages.
2. taxonomy_comment_each_term(): do not call theme() here, just return a render array for the block content. Do not call drupal_add_css() here, use #attached on the render array instead. See https://www.drupal.org/node/930760 and https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
3. "t("Author Name:") . $author[$c_row->uid]": do not concatenate variables to translatable strings, use plcaeholders with t() instead.
4. taxonomy-comments.tpl.php: the $author variable used here is not defined in your hook_theme()? Where is it coming from?
5. taxonomy_comment_form_submit(): the check_plain() call are wrong 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
6. taxonomy_comment_form(): instead of using arg() here you should pass the comment ID in with drupal_get_form().

The t() usage and sanitizing on DB writing operations are blockers right now.

manual review:

1. taxonomy_comment_form_submit(): do not use arg() here. Any data that you need should be coming form submitted $form_state['values] or otherwise from $form_state, which you can prepare in the form building function.
2. taxonomy_comment_each_term(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as comment text this gets printed unsanitized here. You need to apply the proper text format here.
3. taxonomy-comments.tpl.php: do not run check_plain() here, move it to taxonomy_comment_each_term(). If somebody overwrites that template and forgets to sanitize then they have an XSS vulnerability again. That's why the variables should be properly prepared in taxonomy_comment_each_term().

When you submit this for review next time please make sure that all potential XSS vulnerabilities are covered.

Git clone failed for https://git.drupal.org/sandbox/SatyamUpadhyay/2839137.git while invoking http://pareview.sh/pareview/httpsgitdrupalorgsandboxSatyamUpadhyay283913...

Git clone failed. Aborting.

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

Unable to clone the code due to repository not found error.

Hi Satyam ,

Please mention project GIT URL instead of sandbox GIT url.
Because we unable to clone.

Thanks
Ishwar

Multiple Applications

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

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

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

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.